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: Create socket and immediately destroy throws AssertionError #2250

Closed
feross opened this issue Jul 26, 2015 · 7 comments
Closed

net: Create socket and immediately destroy throws AssertionError #2250

feross opened this issue Jul 26, 2015 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem.

Comments

@feross
Copy link
Contributor

feross commented Jul 26, 2015

Starting in v2.3.2, this code which used to work, started to throw an AssertionError:

var net = require('net')

var socket = net.connect(5000, '1.1.1.1') // nothing listening on this address
socket.on('error', function (err) {
  console.log('caught error' + err.message)
})
socket.destroy()

With the following exception:

assert.js:89
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at connect (net.js:778:10)
    at net.js:930:7
    at doNTCallback0 (node.js:408:9)
    at process._tickCallback (node.js:337:13)
    at Function.Module.runMain (module.js:473:11)
    at startup (node.js:117:18)
    at node.js:948:3

I traced the issue back to this PR by @evanlucas, reviewed by @trevnorris and @cjihrig: #2054

@evanlucas
Copy link
Contributor

Ah found it. PR coming shortly

@feross
Copy link
Contributor Author

feross commented Jul 27, 2015

Maybe consider adding a test as well, so there's no regression?

@evanlucas
Copy link
Contributor

Yep, added one. Thanks for the report

@feross
Copy link
Contributor Author

feross commented Jul 27, 2015

Heh, looks like there are two PRs out now :)

@cjihrig
Copy link
Contributor

cjihrig commented Jul 27, 2015

Whoops

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Jul 27, 2015
@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Jul 27, 2015
@trevnorris
Copy link
Contributor

My bad. Didn't take into account a user manually destroying the connection before the connection had completed.

evanlucas added a commit to evanlucas/node that referenced this issue Jul 27, 2015
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
Copy link
Contributor

Fixed in 503b089.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants