Fix a bug that #close for unconnected Socket doesn't detach all watchers #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks! I will check it later. |
`Coolio::IO#close` should detach all own watchers. But `#close` of `Coolio::Socket (< Coolio::IO)` doesn't detach all own watchers when the socket isn't connected yet. `Coolio::IO#close` detaches attached watchers. It uses `Coolio::IO#attached?` to detect an attached watcher. `Coolio::Socket` delegates some operations such as attach and detach to its internal connector before the socket is connected yet. But it doesn't delegate `#attached?`. So `Coolio::IO#close` can't detect an attached watcher by its internal connector. So an attached watcher isn't detached by `#close`. See the committed spec how to reproduce this problem.
|
OK. I'm not in a hurry. |
| @@ -0,0 +1,27 @@ | |||
| require File.expand_path('../spec_helper', __FILE__) | |||
|
|
|||
| describe Coolio::TCPSocket, :env => :win do | |||
There was a problem hiding this comment.
:env => :win is needed?
This test should pass on Windows because doesn't depend on Linux / Mac features like Unix Domain Socket.
There was a problem hiding this comment.
Ah, sorry. I misunderstood.
I thought that :env => :win is for running the description on Windows too.
I didn't run the description on Windows but it will not be needed.
There was a problem hiding this comment.
Yeah. It is very confused name but currently :env => :win is used to skip specs on Windows.
I will change this name after merged this PR.
There was a problem hiding this comment.
Could you remove this?
Other code looks Okay so I can merge PR.
`:env => :win` means that the example doesn't run on Windows. The example should work on Windows. So `:env => :win` should not be specified. Reported by Masahiro Nakagawa. Thanks!!!
repeatedly
added a commit
that referenced
this pull request
May 7, 2014
…watchers Fix a bug that #close for unconnected Socket doesn't detach all watchers
|
Thanks! Good catch 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Coolio::IO#closeshould detach all own watchers. But#closeofCoolio::Socket (< Coolio::IO)doesn't detach all own watchers whenthe socket isn't connected yet.
Coolio::IO#closedetaches attached watchers. It usesCoolio::IO#attached?to detect an attached watcher.Coolio::Socketdelegates some operations such as attach and detachto its internal connector before the socket is connected yet. But it
doesn't delegate
#attached?. SoCoolio::IO#closecan't detect anattached watcher by its internal connector. So an attached watcher
isn't detached by
#close.See the committed spec how to reproduce this problem.