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

http: handle cases where socket.server is null #13578

Closed
wants to merge 2 commits into from
Closed

http: handle cases where socket.server is null #13578

wants to merge 2 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jun 9, 2017 •
edited
Loading

Fixes a regression that caused an error to be thrown when trying to emit the 'timeout' event on the server referenced by socket.server.

Fixes: #13435
Refs: #11926

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

http

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 9, 2017
cjihrig
cjihrig reviewed Jun 9, 2017
View reviewed changes
test/parallel/test-regress-GH-13435.js Outdated
@@ -0,0 +1,37 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance you can give the test a more descriptive name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the format of the existing regression tests. Any suggestion? Can only think of something like test-http-socket-server-not-null which seems as obscure as the current one.

Copy link
Member

Choose a reason for hiding this comment

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

Then perhaps add a comment that describes the purpose of the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does test-cluster-send-net-socket-to-worker-http-server work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe shorten a bit to test-cluster-send-socket-to-worker-server, but it's up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks.

cjihrig
cjihrig reviewed Jun 9, 2017
View reviewed changes
test/parallel/test-regress-GH-13435.js Outdated
} else {
const server = http.createServer();

server.setTimeout(100, common.mustCall((socket) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you achieve the same thing without a timeout? It will probably become flakey in the CI.

Copy link
Member Author

@lpinca lpinca Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, I used the timeout to actually test that socketOnTimeout() is called but checking that socket.server is correctly set should work as well.

@lpinca
http: handle cases where socket.server is null
6f2e806 
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: nodejs#13435
Refs: nodejs#11926
cjihrig
cjihrig approved these changes Jun 9, 2017
View reviewed changes
test/parallel/test-cluster-send-socket-to-worker-http-server.js Outdated
server.on('connection', common.mustCall((socket) => {
assert.strictEqual(socket.server, server);
socket.destroy();
cluster.worker.kill();
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 disconnect() here.

@lpinca
Copy link
Member Author

lpinca commented Jun 10, 2017

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

@lpinca lpinca mentioned this pull request Jun 10, 2017
Merged
@addaleax
Copy link
Member

Landed in ba449f3

@addaleax addaleax closed this Jun 12, 2017
addaleax pushed a commit that referenced this pull request Jun 12, 2017
@lpinca @addaleax
http: handle cases where socket.server is null
ba449f3 
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: #13435
Refs: #11926
PR-URL: #13578
Reviewed-By: Colin Ihrig <[email protected]>
@lpinca lpinca deleted the gh-13435 branch June 12, 2017 12:16
addaleax pushed a commit that referenced this pull request Jun 12, 2017
@lpinca @addaleax
http: handle cases where socket.server is null
fff8a56 
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: #13435
Refs: #11926
PR-URL: #13578
Reviewed-By: Colin Ihrig <[email protected]>
addaleax added a commit that referenced this pull request Jun 12, 2017
@addaleax
2017-06-12, Version 8.1.1 (Current)
2b0a8bd 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
addaleax added a commit that referenced this pull request Jun 12, 2017
@addaleax
2017-06-12, Version 8.1.1 (Current)
e90ad78 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
addaleax added a commit that referenced this pull request Jun 12, 2017
@addaleax
2017-06-13, Version 8.1.1 (Current)
c7f35e2 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
MylesBorins pushed a commit that referenced this pull request Jun 13, 2017
@addaleax @MylesBorins
2017-06-13, Version 8.1.1 (Current)
3e1492a 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
jasnell pushed a commit that referenced this pull request Jun 13, 2017
@addaleax @jasnell
2017-06-13, Version 8.1.1 (Current)
9dc3f93 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@shyser shyser mentioned this pull request Jun 23, 2017
Closed
@MylesBorins
Copy link
Contributor

do we want to backport this to v6.x?

@lpinca
Copy link
Member Author

lpinca commented Jul 17, 2017

@MylesBorins no it's not needed.

@magicode
Copy link

magicode commented Nov 15, 2017 •
edited
Loading

@lpinca

why this

  if (socket.server === null)
    socket.server = this;

this is best

  if (!socket.server)
    socket.server = this;
  //or 
  socket.server = socket.server && this;

i am move from node6 to mode8 and i get undefined error

_http_server.js:393
  var serverTimeout = this.server.emit('timeout', this);
                                  ^

TypeError: Cannot read property 'emit' of undefined
    at TLSSocket.socketOnTimeout (_http_server.js:393:35)

@lpinca
Copy link
Member Author

lpinca commented Nov 15, 2017

@magicode If I remember correctly it was because it is set to null when the socket is created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

passing socket from master to worker causes a timeout issue where socket is null.
7 participants
@lpinca @addaleax @MylesBorins @magicode @jasnell @cjihrig @nodejs-github-bot