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

Passing options object to http.createServer stops server responding #24105

Closed
brokencube opened this issue Nov 5, 2018 · 10 comments
Closed

Passing options object to http.createServer stops server responding #24105

brokencube opened this issue Nov 5, 2018 · 10 comments
Labels
doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem.

Comments

@brokencube
Copy link

brokencube commented Nov 5, 2018

  • Version: node 8.12.0
  • Platform: macOS Sierra 10.12.6 (16G1510)
  • uname -a: Darwin [USERNAME] 16.7.0 Darwin Kernel Version 16.7.0: [DATETIME]; root:xnu-3789.73.14~1/RELEASE_X86_64 x86_64
  • Subsystem: http

Example:

"use strict";
const http = require('http');
const options = {};

const server = http.createServer(options, (req, res) => {
    console.log('got here', req, res);
});
server.listen(4000);
# telnet localhost 4000
GET /

Expected: Console.log of "got here"
Actual: No output.

It also doesn't work is options is set to:

const options = {
  IncomingMessage: http.IncomingMessage,
  ServerResponse: http.ServerResponse
}

Is working as expected in v10.13.0

Using node inspect the difference in execution between v8 and v10 seems to be this line:
_http_common.js:117
return parser.onIncoming(parser.incoming, shouldKeepAlive);

Which should be returning a IncomingMessage object, but instead returns nothing. But that's as far as I could get with this.

Calling without the options object - i.e.:

const server = http.createServer((req, res) => {})

Works as expected

@Trott
Copy link
Member

Trott commented Nov 6, 2018

@nodejs/http

@bnoordhuis bnoordhuis added http Issues or PRs related to the http subsystem. v8.x labels Nov 6, 2018
@oyyd
Copy link
Contributor

oyyd commented Nov 9, 2018

node/doc/api/http.md

Lines 1779 to 1786 in bd8c107

## http.createServer([options][, requestListener])
<!-- YAML
added: v0.1.13
changes:
- version: v9.6.0
pr-url: https://github.com/nodejs/node/pull/15752
description: The `options` argument is supported now.
-->

In fact, The options argument is added in v9.6.0 so that it don't pass requestListener to http.Server in v8.12.0.

@brokencube
Copy link
Author

Looks like it was backported for v8.12.0:
#21593

and from the Release notes from v8.12.0:

add options to http.createServer() (Peter Marton) #15752

@oyyd
Copy link
Contributor

oyyd commented Nov 9, 2018

Yeah, but the code of v8.x-staging seems not:

node/lib/http.js

Lines 33 to 35 in 26d145a

function createServer(requestListener) {
return new Server(requestListener);
}

I'm not sure if I have missed something.

@brokencube
Copy link
Author

brokencube commented Nov 9, 2018

Ah, apparently the http change got backed out of the merge for v8.12.0: #20456 (comment)

@apapirovski
Copy link
Member

As expected given this is not a valid signature for that function.

@brokencube
Copy link
Author

brokencube commented Nov 29, 2018

Well there is still an issue here... either:

  1. That change that was backed out needs to be merged in so that the current 8.x documentation is correct, OR
  2. The documentation for 8.x needs to be updated to have the correct (unchanged) function signature.

@apapirovski
Copy link
Member

@brokencube Thanks for an update. That's helpful in moving this forward. I'm going to reopen and assign the appropriate labels.

@apapirovski apapirovski reopened this Nov 29, 2018
@apapirovski apapirovski added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. doc Issues and PRs related to the documentations. labels Nov 29, 2018
aautio added a commit to aautio/node that referenced this issue Dec 5, 2018
@aautio
Copy link

aautio commented Dec 5, 2018

I've submitted #24856 as a fix for the documentation.

Edit: I made the first pull request in a wrong branch. Here is a new one: #24869

aautio added a commit to aautio/node that referenced this issue Dec 6, 2018
MylesBorins pushed a commit that referenced this issue Dec 10, 2018
Fixes: #24105

PR-URL: #24869
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins
Copy link
Contributor

landed in v8.x-staging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants