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

test: fix net-server-max-connections-close-makes-more-available #1881

Closed
wants to merge 1 commit into from
Closed

test: fix net-server-max-connections-close-makes-more-available #1881

wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

  • Handle connection 'error' event so the test ends.

@santigimeno
Copy link
Member Author

Before this change I was getting this error:

creating connection 0
sending message: 0
received message: 0
connection 0 received response
creating connection 1
sending message: 1
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: read ECONNRESET
    at exports._errnoException (util.js:846:11)
    at TCP.onread (net.js:540:26)

Not sure if this change is correct though, as I'm not familiar with Promises

@targos
Copy link
Member

targos commented Jun 3, 2015

I am not getting this error :

➜  io.js git:(master) ✗ ./iojs test/parallel/test-net-server-max-connections-close-makes-more-available.js
creating connection 0
sending message: 0
received message: 0
connection 0 received response
creating connection 1
sending message: 1
ending 1
closing connection 0
ending 0
creating connection 2
sending message: 2
received message: 2
connection 2 received response
creating connection 3
sending message: 3
ending 3
closing connection 2
ending 2

Is this error event supposed to be emitted ?

@santigimeno
Copy link
Member Author

I'm getting this error consistently in a FreeBSD 10.1 box

@bnoordhuis
Copy link
Member

Refs #1855 and /cc @brendanashworth @Trott

- Handle connection 'error' event so the test ends.
@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Jun 3, 2015
failed.push(index);
resolve();
});

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a check to make sure the err.code is ECONNRESET just to make sure some other problem doesn't mask an issue here?

@Trott
Copy link
Member

Trott commented Jun 3, 2015

This isn't happening for me (just built from master on OS X). If some setups send ECONNRESET and others just silently drop the connection, that's probably a bigger bug than just tweaking this test, yeah? (Although I suppose there may be OS-specific limitations on what types of errors can be retrieved. That doesn't seem likely to be an issue with this, though.)

@silverwind
Copy link
Contributor

This test failed on a bunch of platforms before: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/758/

@brendanashworth
Copy link
Contributor

I should have run the test on the CI, sorry for that. But there does seem to be an issue if an ECONNRESET is thrown on some platforms and not others.

@Trott
Copy link
Member

Trott commented Jun 3, 2015

This version in this pull request is failing for me on OS X:

assert.js:88
  throw new assert.AssertionError({
        ^
AssertionError: [] deepEqual [ 1, 3 ]
    at process.<anonymous> (/Users/trott/io.js/test/parallel/test-net-server-max-connections-close-makes-more-available.js:87:10)
    at emitOne (events.js:82:20)
    at process.emit (events.js:169:7)

This would seem to suggest that whether or not an error event is triggered when a connection is rejected may be platform-dependent. It would also seem to me that the FreeBSD behavior is correct (and the test should more resemble what's in this pull request) and the OS X behavior is a bug.

@silverwind
Copy link
Contributor

@santigimeno
Copy link
Member Author

@Trott I have applied your suggestions
Also I have removed the failed check so it passes in other platforms, but I agree that it looks like a bug

@brendanashworth
Copy link
Contributor

CI is happy, I'm happy.

@Trott
Copy link
Member

Trott commented Jun 3, 2015

I created an issue for the not-triggering-ECONNRESET thing.

@brendanashworth
Copy link
Contributor

We should get this in. LGTM.

@Trott
Copy link
Member

Trott commented Jun 3, 2015

LGTM

(Not sure if my LGTM counts for anything, but if it doesn't count for nothing, then there it is.)

brendanashworth pushed a commit that referenced this pull request Jun 3, 2015
Fixes net-server-max-connections-close-makes-more-available
Handles connection 'error' event so the test ends.

PR-URL: #1881
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@brendanashworth
Copy link
Contributor

Thanks everyone, landed in a804026. (I had to fix the commit log a little bit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants