-
Notifications
You must be signed in to change notification settings - Fork 471
don't hold references to unowned DispatchData objects (SR-3628) #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
borrowedData was added to Data to compensate for memory leaks that I think were related to iteration. I will go back and look at that PR / SR to see how this proposed change interacts with that. |
ok. This change makes sense to me. When I "fixed" the over-release problem of SR-2656 by introducing borrowedData: in #175 I didn't account for when the __DispatchData that was borrowing the dispatch_data_t outlived the "real" owner. I was too focused on trying to avoid the overhead of an "extra" retain/release operation. But, it really can't be avoided (as shown by the test case for this bug). |
@@ -188,21 +188,20 @@ extension DispatchSource : DispatchSourceProcess, | |||
|
|||
internal class __DispatchData : DispatchObject { | |||
internal let __wrapped:dispatch_data_t | |||
internal let __owned:Bool | |||
|
|||
final internal override func wrapped() -> dispatch_object_t { | |||
return unsafeBitCast(__wrapped, to: dispatch_object_t.self) | |||
} | |||
|
|||
internal init(data:dispatch_data_t, owned:Bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that owned
tag should really be transferOwnership: Bool
but the patch LGTM and I should have realized when we reviewed the earlier patch :/
@swift-ci please test |
build seems healthy to me but then failed because of
|
@swift-ci please test |
@dgrove-oss I'm not 100% sure, do we need this for another branch than master? we want that for whatever the next swift 3 update will be. |
We should get it into the swift-3.1-branch. I don't think it is worth putting into the swift-3.0-branch. |
@dgrove-oss sure, on it! |
don't hold references to unowned DispatchData objects (SR-3628) Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>
In
DispatchIO.read
, thedispatch_data_t
object received fromdispatch_io_read
will be released once the block passed todispatch_io_read
returns.In the Swift overlay (
src/swift/IO.swift
) the code marks that data object correctly as "borrowedData"that however leads to use-after-frees if the Swift
DispatchData
is kept around for longer.The change I made will
retain
the underlyingdispatch_data_t
if it's "borrowed" which I believe is correct and fixes SR-3628.