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

Fix CI on Node 7.7.2 #3262

Merged
merged 1 commit into from
Mar 9, 2017
Merged

Fix CI on Node 7.7.2 #3262

merged 1 commit into from
Mar 9, 2017

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Mar 9, 2017

Description

Add workaround for what seems to be a Node 7.7.2 regression: net.createConnection(port) is failing with the following error, see e.g. https://travis-ci.org/strongloop/loopback/jobs/209262349:

  1) loopback application pauses request stream during authentication:
     Uncaught TypeError: "listener" argument must be a function
    at Socket.connect (net.js:943:10)
    at Socket.connect (node_modules/async-listener/index.js:76:27)
    at Object.exports.connect.exports.createConnection (net.js:76:35)
    at sendHttpRequestInOnePacket (test/integration.test.js:77:24)
    at Server.<anonymous> (test/integration.test.js:19:7)
    at emitListeningNT (net.js:1302:10)
    at node_modules/async-listener/glue.js:188:31
    at _combinedTickCallback (internal/process/next_tick.js:77:11)
    at process._tickDomainCallback [as _tickCallback] (internal/process/next_tick.js:128:9)

The same code passes on Node 7.7.1, see e.g. https://travis-ci.org/strongloop/loopback/jobs/209009746.

My proposed workaround is to explicitly pass a no-op callback argument:

net.createConnection(port, ()=>{});

Strangely enough, the problem CANNOT be reproduced when the failing test is run in isolation via mocha test/integration.test.js. To reproduce it, one has to add few more tests making HTTP requests, e.g. via

$ mocha -R dot test/access-control.integration.js test/integration.test.js

From looking at recent Node.js changes, it seems the problem was introduced by nodejs/node@b56e851 from nodejs/node#11667.

@joyeecheung @mcollina @jasnell you have authored/reviewed that patch, do you happen to have any insights on what may be wrong here?

cc @sam-github

@bajtos bajtos added #wip review and removed #wip labels Mar 9, 2017
@bajtos bajtos changed the title Fix CI on Node 7.7.2Add workaround for Node 7.7.2 regression Fix CI on Node 7.7.2 Mar 9, 2017
@bajtos bajtos requested a review from superkhau March 9, 2017 09:29
@joyeecheung
Copy link

joyeecheung commented Mar 9, 2017

It is indeed a regression from Node's side. Should be fixed by nodejs/node#11762

@bajtos
Copy link
Member Author

bajtos commented Mar 9, 2017

@joyeecheung lovely, thank you for chiming in and pointing us to the pull request fixing the regression. 👍 ❤️

In this light, I will rework my pull request to simply lock Travis CI config to 7.7.1 until a fixed version of Node.js is available.

@bajtos bajtos merged commit 3a93167 into master Mar 9, 2017
@bajtos bajtos deleted the fix/ci-on-node-7.7.2 branch March 9, 2017 12:32
@bajtos bajtos removed the review label Mar 9, 2017
@superkhau
Copy link
Contributor

LGTM

@italoacasas
Copy link

@bajtos
Copy link
Member Author

bajtos commented Mar 15, 2017

@italoacasas thanks! I opened a pull request to revert our Travis CI config back - #3285

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.

4 participants