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

net: return if not _connecting #2251

Merged
merged 1 commit into from
Jul 27, 2015

Conversation

evanlucas
Copy link
Contributor

Fixes regression introduced in af249fa.

With connect being deferred to the next tick, Socket.destroy could be
called before connect. Socket.destroy sets _connecting to false which
would cause an assertion error.

Fixes: #2250

@evanlucas evanlucas added confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem. labels Jul 27, 2015
const assert = require('assert');
const net = require('net');

var socket = net.connect(5000, '1.1.1.1', assert.fail);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use common.PORT and common.localhostIPv4 for the port and host here.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 27, 2015

Two small nits. Can you also come up with a slightly more descriptive first line of the commit message? Other than that, LGTM.

@evanlucas evanlucas force-pushed the fixconnectingdestroy branch from bfc3ad1 to 0ef648e Compare July 27, 2015 00:46
@evanlucas
Copy link
Contributor Author

Ok, updated. I tried to start a CI job on it, but it seems to not be working.

@evanlucas
Copy link
Contributor Author

Weird...looks like the current build has been running for 2 days now?

ping @Fishrock123

@Fishrock123
Copy link
Contributor

^ ping @nodejs/build :)

@Fishrock123 Fishrock123 removed the confirmed-bug Issues with confirmed bugs. label Jul 27, 2015
@Fishrock123
Copy link
Contributor

LGTM if the tests are happy. Tagged the original issue with confirmed-bug.

@Fishrock123 Fishrock123 added this to the 2.5.0 milestone Jul 27, 2015
@orangemocha
Copy link
Contributor

Weird...looks like the current build has been running for 2 days now?

Looking into it... one ubuntu slave is stuck

@@ -927,6 +927,8 @@ function lookupAndConnect(self, options) {
var addressType = exports.isIP(host);
if (addressType) {
process.nextTick(function() {
if (!self._connecting)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an observation. We can actually avoid the empty statement here if we flip the condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea. We could go the route @cjihrig went just use if (self._connecting) ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, just checked the closed PR :D

@evanlucas evanlucas force-pushed the fixconnectingdestroy branch from 0ef648e to b516a61 Compare July 27, 2015 19:01
@evanlucas
Copy link
Contributor Author

@evanlucas
Copy link
Contributor Author

Will land after CI finishes provided it is happy.

@thefourtheye
Copy link
Contributor

LGTMT 👍

@Fishrock123
Copy link
Contributor

@evanlucas CI looks good.

Fixes regression introduced in af249fa.

With connect being deferred to the next tick, Socket.destroy could be
called before connect. Socket.destroy sets _connecting to false which
would cause an assertion error.

Fixes: nodejs#2250
PR-URL: nodejs#2251
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@evanlucas evanlucas force-pushed the fixconnectingdestroy branch from b516a61 to 503b089 Compare July 27, 2015 21:31
@evanlucas evanlucas closed this Jul 27, 2015
@evanlucas evanlucas deleted the fixconnectingdestroy branch July 27, 2015 21:31
@evanlucas evanlucas merged commit 503b089 into nodejs:master Jul 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants