HTTP/1 createServer() options to pass custom Request and Response classes #15752

hekike · 2017-10-03T07:09:06Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http
https
http2 (http fallback)

Related PR

HTTP/2 createServer() options to pass custom Request and Response classes #15560 (check out conversation)

mscdex · 2017-10-03T07:56:53Z

doc/api/http.md

@@ -1720,10 +1720,17 @@ A collection of all the standard HTTP response status codes, and the
 short description of each.  For example, `http.STATUS_CODES[404] === 'Not
 Found'`.

-## http.createServer([requestListener])
+## http.createServer([options][requestListener])


Missing comma

mscdex · 2017-10-03T07:57:23Z

doc/api/http.md

 <!-- YAML
 added: v0.1.13
 -->
+- `options` {Object}
+  * `IncomingMessage` {http.IncomingMessage} Specifies the IncomingMessage class to
+    used. Useful for extending the original `IncomingMessage`.


Missing 'be' at the beginning?

mscdex · 2017-10-03T07:57:41Z

doc/api/http.md

+  * `IncomingMessage` {http.IncomingMessage} Specifies the IncomingMessage class to
+    used. Useful for extending the original `IncomingMessage`.
+    Defaults to: `Http2ServerRequest`
+  * `ServerResponse` {htt.ServerResponse} Specifies the ServerResponse class to


s/htt.ServerResponse/http.ServerResponse/

mscdex · 2017-10-03T07:57:53Z

doc/api/http.md

+    used. Useful for extending the original `IncomingMessage`.
+    Defaults to: `Http2ServerRequest`
+  * `ServerResponse` {htt.ServerResponse} Specifies the ServerResponse class to
+    used. Useful for extending the original `ServerResponse`.


Ditto about missing 'be'

mscdex · 2017-10-03T07:58:27Z

doc/api/http.md

+- `options` {Object}
+  * `IncomingMessage` {http.IncomingMessage} Specifies the IncomingMessage class to
+    used. Useful for extending the original `IncomingMessage`.
+    Defaults to: `Http2ServerRequest`


I don't understand both of these defaults. This documentation is for http/1.x.

mscdex · 2017-10-03T08:00:02Z

lib/_http_server.js


-function Server(requestListener) {
-  if (!(this instanceof Server)) return new Server(requestListener);
+  if (!(this instanceof Server)) return new Server(options, requestListener);


This should remain the first line to avoid executing the same code above twice in the case of no new.

Don't we need to initialize options in the case of no new?

@hekike yes but it can be done after that, see https.js for example.

mscdex · 2017-10-03T08:01:49Z

Have you ran the appropriate http1 benchmarks before and after these changes and compared the results? If so, what were your findings?

lpinca · 2017-10-03T07:27:59Z

doc/api/http.md

+  * `IncomingMessage` {http.IncomingMessage} Specifies the IncomingMessage class to
+    used. Useful for extending the original `IncomingMessage`.
+    Defaults to: `Http2ServerRequest`
+  * `ServerResponse` {htt.ServerResponse} Specifies the ServerResponse class to


Typo: htt -> http

lpinca · 2017-10-03T07:31:57Z

doc/api/http.md

@@ -1720,10 +1720,17 @@ A collection of all the standard HTTP response status codes, and the
 short description of each.  For example, `http.STATUS_CODES[404] === 'Not
 Found'`.

-## http.createServer([requestListener])
+## http.createServer([options][requestListener])


Nit: missing comma, [options][, requestListener].

lpinca · 2017-10-03T07:32:35Z

doc/api/http.md

 <!-- YAML
 added: v0.1.13
 -->
+- `options` {Object}
+  * `IncomingMessage` {http.IncomingMessage} Specifies the IncomingMessage class to
+    used. Useful for extending the original `IncomingMessage`.


Typo: used -> use

lpinca · 2017-10-03T07:32:59Z

doc/api/http.md

+    used. Useful for extending the original `IncomingMessage`.
+    Defaults to: `Http2ServerRequest`
+  * `ServerResponse` {htt.ServerResponse} Specifies the ServerResponse class to
+    used. Useful for extending the original `ServerResponse`.


Typo: used -> use

lpinca · 2017-10-03T07:36:44Z

doc/api/http.md

+- `options` {Object}
+  * `IncomingMessage` {http.IncomingMessage} Specifies the IncomingMessage class to
+    used. Useful for extending the original `IncomingMessage`.
+    Defaults to: `Http2ServerRequest`


It should be http.IncomingMessage.

lpinca · 2017-10-03T07:37:45Z

doc/api/http.md

+    Defaults to: `Http2ServerRequest`
+  * `ServerResponse` {htt.ServerResponse} Specifies the ServerResponse class to
+    used. Useful for extending the original `ServerResponse`.
+    Defaults to: `Http2ServerResponse`


It should be http.ServerResponse.

lpinca · 2017-10-03T07:41:41Z

doc/api/https.md

@@ -52,7 +52,8 @@ See [`http.Server#keepAliveTimeout`][].
 <!-- YAML
 added: v0.3.4
 -->
- `options` {Object} Accepts `options` from [`tls.createServer()`][] and [`tls.createSecureContext()`][].
+- `options` {Object} Accepts `options` from [`tls.createServer()`][],
+ [`tls.createSecureContext()`][] and [`http.createServer()`][].


We should add a link at the bottom for http.createServer().

lpinca · 2017-10-03T07:55:02Z

lib/_http_server.js

+    requestListener = options;
+    options = {};
+  } else if (options == null || typeof options === 'object') {
+    options = options || {};


I would use util._extend() or Object.assign() like it's done in https.js.

apapirovski · 2017-10-03T13:52:58Z

lib/_http_server.js

@@ -560,7 +570,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
    }
  }

-  var res = new ServerResponse(req);
+  var res = new socket.server.ServerResponse(req);


Could this just be server.ServerResponse(req) instead of socket.server[...]? The function seems to have an in-scope server.

I tried but we don't have the server here, only under the socket.server:

node/lib/_http_common.js

Line 77 in 42a2a9b

parser.incoming = new IncomingMessage(parser.socket);

I could swear it's available

function parserOnIncoming(server, socket, state, req, keepAlive) { resetSocketTimeout(server, socket, state);

It's being used a few places in that function to emit, etc.

@hekike it's in the function definition, it should be populated. Can you update?

@mcollina @apapirovski thanks, fixed!

apapirovski · 2017-10-03T13:56:37Z

lib/_http_server.js

+  }
+
+  this.IncomingMessage = options.IncomingMessage || IncomingMessage;
+  this.ServerResponse = options.ServerResponse || ServerResponse;


I don't have an answer to this but I would like to flag the fact that this means that IncomingMessage & ServerResponse can be altered on the fly, rather than solely through options. It's possible this isn't desirable and we should instead use Symbols, or maybe it is desirable and we should document it as such.

This is also strictly different than the proposed http2 implementation where those classes can't be modified on the fly as they're bound when a request listener is created.

I would actually prefer them to be symbols and be private, if there there are no performance regressions.

@apapirovski @mcollina please check out my latest commit, I made them private.
I'm going to split them into http1/2 commits after you reviewed it.

apapirovski · 2017-10-03T14:04:22Z

lib/_http_common.js

@@ -74,7 +74,11 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
    parser._url = '';
  }

-  parser.incoming = new IncomingMessage(parser.socket);
+  // Parser is also used by http client
+  var ParserIncomingMessage = parser.socket.server ?


This a super minor nit but ideally we would check parser.socket.server !== null. (We know that it's always either null or a Server.)

I checked it and it can be undefined, for example:

TypeError: Cannot read property 'IncomingMessage' of undefined at HTTPParser.parserOnHeadersComplete (_http_common.js:79:26) at TLSSocket.socketOnData (_http_client.js:437:20) at emitOne (events.js:115:13) at TLSSocket.emit (events.js:210:7) at addChunk (_stream_readable.js:264:12) at readableAddChunk (_stream_readable.js:251:11) at TLSSocket.Readable.push (_stream_readable.js:209:10) at TLSWrap.onread (net.js:591:20) Command: out/Release/node /Users/pmarton/Documents/dev/node/test/parallel/test-async-wrap-GH13045.js

Thanks for checking!

apapirovski · 2017-10-03T14:09:01Z

test/parallel/test-https-server-options-incoming-message.js

@@ -0,0 +1,39 @@
+'use strict';
+const common = require('../common');
+const fs = require('fs');


Could you add (right after common is required):

if (!common.hasCrypto) common.skip('missing crypto');

Thanks!

apapirovski · 2017-10-03T14:09:23Z

test/parallel/test-https-server-options-server-response.js

@@ -0,0 +1,35 @@
+'use strict';
+const common = require('../common');
+const fs = require('fs');


Same as above, could you add:

if (!common.hasCrypto) common.skip('missing crypto');

Thanks!

apapirovski · 2017-10-03T14:11:07Z

Looks great overall! My comments are just super minor nits. Thanks for doing this work @hekike (and coming up with the solution in the first place)!

hekike · 2017-10-05T10:17:56Z

@apapirovski I fixed your comments.

Also, I realized that http2 fallback to http should take a place in this PR.
I moved it here.

mcollina

LGTM

mcollina · 2017-10-06T20:52:23Z

@hekike can you split the http2 bits into a separate commit, so we can backport the http1 bit easily?

hekike · 2017-10-09T07:17:09Z

@mcollina sure, done!

mcollina · 2017-10-09T08:35:13Z

test/parallel/test-http-server-options-incoming-message.js

+
+const assert = require('assert');
+const http = require('http');
+


can you add a quick comment describing what this test is about?

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

This adds the optional options argument to `http.createServer()`. It contains two options: the `IncomingMessage` and `ServerReponse` option. Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

This adds the optional options argument to `http.createServer()`. It contains two options: the `IncomingMessage` and `ServerReponse` option. Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

MylesBorins · 2018-06-20T15:19:11Z

@nodejs/lts should we consider backporting to 8.x

should come with #18605

This adds the optional options argument to `http.createServer()`. It contains two options: the `IncomingMessage` and `ServerReponse` option. Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

8.12 Changelog https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V8.md#8.12.0 Specifically [01dc646] - http: add options to http.createServer() nodejs#15752

8.12 Changelog https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V8.md#8.12.0 Specifically [01dc646] - http: add options to http.createServer() #15752 PR-URL: #25001 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

nodejs-github-bot added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Oct 3, 2017

hekike changed the title ~~http: create server IncomingMessage and ServerResponse options~~ HTTP/1 createServer() options to pass custom Request and Response classes Oct 3, 2017

hekike mentioned this pull request Oct 3, 2017

HTTP/2 createServer() options to pass custom Request and Response classes #15560

Closed

4 tasks

mscdex reviewed Oct 3, 2017

View reviewed changes

lpinca reviewed Oct 3, 2017

View reviewed changes

hekike force-pushed the feat/http1-request-response-extend branch from 1353cd0 to 7e51e32 Compare October 3, 2017 08:24

jsumners mentioned this pull request Oct 3, 2017

Possible custom Request and Reply objects? fastify/fastify#327

Open

apapirovski reviewed Oct 3, 2017

View reviewed changes

mcollina mentioned this pull request Oct 4, 2017

Add context argument to emitter #15763

Closed

hekike force-pushed the feat/http1-request-response-extend branch 2 times, most recently from e6d1f5c to c79af6a Compare October 5, 2017 09:36

mcollina approved these changes Oct 6, 2017

View reviewed changes

hekike force-pushed the feat/http1-request-response-extend branch from 886a828 to e0af1eb Compare October 9, 2017 07:16

hekike force-pushed the feat/http1-request-response-extend branch from e0af1eb to f75353c Compare October 9, 2017 08:23

mcollina reviewed Oct 9, 2017

View reviewed changes

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

MylesBorins added the backported-to-v8.x label Aug 7, 2018

brokencube mentioned this pull request Nov 9, 2018

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

Closed

benswinburne mentioned this pull request Dec 12, 2018

doc: corrected version http.createServer options addition #25001

Closed

4 tasks

Ayase-252 mentioned this pull request Jun 10, 2021

http,https: align options of https's server with http's #38992

Closed

callahad mentioned this pull request Jan 5, 2024

nodejs unit-http misses options in createServer function nginx/unit#1043

Closed


		const assert = require('assert');
		const http = require('http');

HTTP/1 createServer() options to pass custom Request and Response classes #15752

HTTP/1 createServer() options to pass custom Request and Response classes #15752

Conversation

hekike commented Oct 3, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Related PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Oct 3, 2017

hekike commented Oct 5, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Oct 6, 2017

hekike commented Oct 9, 2017

Choose a reason for hiding this comment

MylesBorins commented Jun 20, 2018

hekike commented Oct 3, 2017 •

edited

Loading

mscdex commented Oct 3, 2017 •

edited

Loading

apapirovski Oct 3, 2017 •

edited

Loading

apapirovski Oct 3, 2017 •

edited

Loading

apapirovski Oct 3, 2017 •

edited

Loading

apapirovski Oct 3, 2017 •

edited

Loading