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

MORAY-324: client sometimes hang after a call to close() #14

Closed
wants to merge 1 commit into from
Closed

MORAY-324: client sometimes hang after a call to close() #14

wants to merge 1 commit into from

Conversation

misterdjules
Copy link
Contributor

Allows Client.prototype.close to actually abort the connection if it hasn't been established yet. Otherwise, this can lead to the issue described at https://smartos.org/bugview/MORAY-324.

Note that the newly added test will make the process running the tests suite hang when running with the current implementation. A timeout could be added to make the test fail instead.

/cc @pfmooney

@misterdjules
Copy link
Contributor Author

@pfmooney Updated per our discussion. One thing is missing compared to what we discussed: the newly added test does not check that the number of connections is 0 when client2's 'close' event is handled.

The reason is that net.Server.getConnections returns a value that is updated when the server side of the connection handles the 'close' event, which can happen in another tick of the libuv event loop. We talked about doing that check from within a timer using a hardcoded delay. I don't like using hardcoded timeouts, which is why I haven't added that to the test, but if you think it's worth it I'm happy to add that.

@misterdjules
Copy link
Contributor Author

Updated PR tested on OS X, Smartos with the following versions:

root@dev ~/node-fast]# uname -a
SunOS dev 5.11 joyent_20150219T102159Z i86pc i386 i86pc Solaris
[root@dev ~/node-fast]# cat /etc/pkgsrc_version 
release: 2014Q4
architecture: x86_64
[root@dev ~/node-fast]# node --version
v0.10.40
[root@dev ~/node-fast]#

and Ubuntu 12.04 with the following versions:

jgilli@ubuntu:~/dev/node-fast$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 12.04.4 LTS
Release:    12.04
Codename:   precise
jgilli@ubuntu:~/dev/node-fast$ uname -a
Linux ubuntu 3.13.0-62-generic #102~precise1-Ubuntu SMP Wed Aug 12 14:09:54 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
jgilli@ubuntu:~/dev/node-fast$ node --version
v0.10.40
jgilli@ubuntu:~/dev/node-fast$

No regression found.

@pfmooney
Copy link
Contributor

Merged in 3e4ad85.

Thanks

@pfmooney pfmooney closed this Jan 20, 2016
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.

2 participants