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.

Sign up for GitHub

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

Jump to bottom

https: add missing localPort while create socket #24554

Closed
wants to merge 1 commit into from
Closed

https: add missing localPort while create socket #24554

wants to merge 1 commit into from

Conversation

leeight
Copy link
Contributor

@leeight leeight commented Nov 21, 2018 •
edited
Loading

In _tls_wrap.js while calling socket.connect the localPort was
missing, restore it.

Fix: #24543

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Nov 21, 2018
@leeight
Copy link
Contributor Author

leeight commented Nov 21, 2018

Travis CI failed due to github api rate limit.

+PR_COMMITS='{
  "message": "API rate limit exceeded for 35.188.73.34. (But here'\''s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
  "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}'
++node -p 'JSON.parse(process.argv[1])[0].url' '{
  "message": "API rate limit exceeded for 35.188.73.34. (But here'\''s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
  "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}'

@leeight leeight force-pushed the fix-24543 branch 3 times, most recently from 13d69be to 9725da0 Compare November 22, 2018 05:31
@mscdex
Copy link
Contributor

mscdex commented Nov 23, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18891/

@leeight
Copy link
Contributor Author

leeight commented Nov 23, 2018

node-test-commit-plinux failed due to an jenkins exception

10:50:13 FATAL: command execution failed
10:50:13 java.nio.channels.ClosedChannelException
10:50:13 	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:209)
10:50:13 	at org.jenkinsci.remoting.protocol.ApplicationLayer.onRecvClosed(ApplicationLayer.java:222)
10:50:13 	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.onRecvClosed(ProtocolStack.java:832)
10:50:13 	at org.jenkinsci.remoting.protocol.FilterLayer.onRecvClosed(FilterLayer.java:287)
10:50:13 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.onRecvClosed(SSLEngineFilterLayer.java:181)
10:50:13 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.switchToNoSecure(SSLEngineFilterLayer.java:283)
10:50:13 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processWrite(SSLEngineFilterLayer.java:503)
10:50:13 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processQueuedWrites(SSLEngineFilterLayer.java:248)
10:50:13 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doSend(SSLEngineFilterLayer.java:200)
10:50:13 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doCloseSend(SSLEngineFilterLayer.java:213)
10:50:13 	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.doCloseSend(ProtocolStack.java:800)
10:50:13 	at org.jenkinsci.remoting.protocol.ApplicationLayer.doCloseWrite(ApplicationLayer.java:173)
10:50:13 	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer$ByteBufferCommandTransport.closeWrite(ChannelApplicationLayer.java:314)

@leeight
https: add missing localPort while create socket
c937e38 
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

Fix: nodejs#24543
@leeight
Copy link
Contributor Author

leeight commented Nov 24, 2018

@mscdex @lpinca

@Trott
Copy link
Member

Trott commented Dec 1, 2018

Brand new fresh CI: https://ci.nodejs.org/job/node-test-pull-request/19110/

@Trott
Copy link
Member

Trott commented Feb 21, 2019

Hmmm...this stalled somehow.

CI: https://ci.nodejs.org/job/node-test-pull-request/20940/

/ping @nodejs/http for reviews

lpinca
lpinca approved these changes Feb 21, 2019
View reviewed changes
test/parallel/test-https-connect-localport.js
const https = require('https');
const assert = require('assert');

{
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think there is no reason to use this block.

@Trott
Copy link
Member

Trott commented Feb 21, 2019

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20941/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2019
@Trott
Copy link
Member

Trott commented Feb 21, 2019

Resume Build CI again: https://ci.nodejs.org/job/node-test-pull-request/20946/

macOS is acting up. 😞

@lpinca lpinca added the https Issues or PRs related to the https subsystem. label Mar 1, 2019
@lpinca
Copy link
Member

lpinca commented Mar 1, 2019

New CI: https://ci.nodejs.org/job/node-test-pull-request/21079/

@BridgeAR
Copy link
Member

BridgeAR commented Mar 4, 2019 •
edited
Loading

Resumed CI https://ci.nodejs.org/job/node-test-pull-request/21174/ ✔️

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 5, 2019
@leeight @BridgeAR
https: add missing localPort while create socket
c8e0c09 
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

PR-URL: nodejs#24554
Fixes: nodejs#24543
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Landed in c8e0c09 🎉

@BridgeAR BridgeAR closed this Mar 5, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@leeight @BridgeAR
https: add missing localPort while create socket
4b6c653 
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

PR-URL: nodejs#24554
Fixes: nodejs#24543
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
@leeight @BethGriggs
https: add missing localPort while create socket
b7b08d1 
In `_tls_wrap.js` while calling `socket.connect` the `localPort` was
missing, restore it.

PR-URL: #24554
Fixes: #24543
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. https Issues or PRs related to the https subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

createConnection option not work for https module
7 participants
@leeight @mscdex @Trott @lpinca @BridgeAR @jasnell @nodejs-github-bot