Skip to content
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

Don't try to wrap socket "onread" when socket is already closed. #15

Closed
wants to merge 1 commit into from

Conversation

apparentlymart
Copy link
Contributor

In order to wrap socket._handle.onread, we hook into the server's 'connection' event with the aim of wrapping the onread function before request handling begins.

However, it is possible that other 'connection' event handlers will run before ours, and they may close the connection before we get a chance to run. In that case, socket._handle is already gone and there is no
longer any onread function for us to wrap, so we just do nothing.

I believe this resolves issue #13.

@othiym23
Copy link
Owner

Thanks so much! I'll get this merged and pushed this weekend!

@othiym23
Copy link
Owner

In the meantime, do you have a test for the issue this is addressing?

In order to wrap socket._handle.onread, we hook into the server's
'connection' event with the aim of wrapping the onread function before
request handling begins.

However, it is possible that other 'connection' event handlers will run
before ours, and they may close the connection before we get a chance
to run. In that case, socket._handle is already gone and there is no
longer any onread function for us to wrap, so we just do nothing.
@apparentlymart
Copy link
Contributor Author

I've updated the change with a new test. I think I've followed the style of the other tests but please let me know if anything doesn't look right.

Confirmed that the test fails before applying my change, and succeeds afterwards.

@othiym23
Copy link
Owner

Landed in 23ad97a. Thanks for your help! It's amazing how careful you have to be in so few lines of code. Also: I apologize for sitting on this for so long. It completely slipped my mind!

@othiym23 othiym23 closed this Jul 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants