Skip to content

Commit

Permalink
Improve close methods for SingleAsyncWork
Browse files Browse the repository at this point in the history
close(t::Timer) also works for SingleAsyncWork, so let that method take both.
Further, the close hook for SingleAsyncWork needs to remove it from the preservation dict.
  • Loading branch information
Keno committed Mar 23, 2015
1 parent 1643a06 commit 67ed8d1
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions base/stream.jl
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ type Timer <: AsyncWork
end
end

close(t::Timer) = ccall(:jl_close_uv,Void,(Ptr{Void},),t.handle)
close(t::AsyncWork) = ccall(:jl_close_uv,Void,(Ptr{Void},),t.handle)

function _uv_hook_close(uv::Union(AsyncStream,UVServer))
uv.handle = C_NULL
Expand All @@ -474,7 +474,8 @@ function _uv_hook_close(uv::Union(AsyncStream,UVServer))
try notify(uv.readnotify) end
try notify(uv.connectnotify) end
end
_uv_hook_close(uv::AsyncWork) = (uv.handle = C_NULL; nothing)
_uv_hook_close(uv::Timer) = (uv.handle = C_NULL; nothing)
_uv_hook_close(uv::SingleAsyncWork) = (uv.handle = C_NULL; unpreserve_handle(uv); nothing)

# This serves as a common callback for all async classes
function _uv_hook_asynccb(async::AsyncWork)
Expand Down

10 comments on commit 67ed8d1

@jakebolewski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the handles were being preserved, is this the source of the memory leak in JuliaInterop/ZMQ.jl#76?

@Keno
Copy link
Member Author

@Keno Keno commented on 67ed8d1 Mar 23, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is backported, and may justify a prompt 0.3.7 release. Cc: @JuliaBackports @tkelman

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tagged 0.3.7 a few hours ago. We haven't posted the binaries yet, but the git history is there.

@staticfloat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.3.7 hasn't been officially released yet. Is this critical enough for us to retag, rebuild binaries and then release?

On an unrelated note, it amuses me that the three verbs start with 're-' but only two of them imply a repetition of work.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-referencing JuliaInterop/ZMQ.jl#76 - I don't like deleting a tag that's been public in the history for ~12 hours, I'd rather just supersede it with a new one and go straight to 0.3.8. A very short march, potentially.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is backported

No not yet. @Keno should it be?

@tkelman
Copy link
Contributor

@tkelman tkelman commented on 67ed8d1 Apr 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump. Again, I don't want to backport this until I get confirmation that it should be backported.

@Keno
Copy link
Member Author

@Keno Keno commented on 67ed8d1 Apr 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's necessary, and since packages will probably want to support older 0.3 versions too, it would complicate the version checking.

@tkelman
Copy link
Contributor

@tkelman tkelman commented on 67ed8d1 Apr 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay great, thanks Keno

Please sign in to comment.