HTTP/1 createServer() options to pass custom Request and Response classes by hekike · Pull Request #15752 · nodejs/node · GitHub

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)

Missing comma

Missing 'be' at the beginning?

s/htt.ServerResponse/http.ServerResponse/

Ditto about missing 'be'

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

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.

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

Typo: htt -> http

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

Typo: used -> use

It should be http.IncomingMessage.

It should be http.ServerResponse.

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

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

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!

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.

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!

Could you add (right after common is required):

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

Thanks!

Same as above, could you add:

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

Thanks!

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)!

@apapirovski I fixed your comments.

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

LGTM

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

@mcollina sure, done!

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

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

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 <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

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 <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

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 <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

@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 <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

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 <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

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

Closed

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

HTTP/1 createServer() options to pass custom Request and Response classes by hekike · Pull Request #15752 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

HTTP/1 createServer() options to pass custom Request and Response classes#15752hekike wants to merge 2 commits intonodejs:masternodejs/node:masterfrom hekike:feat/http1-request-response-extendhekike/node:feat/http1-request-response-extendCopy head branch name to clipboard

Conversation

hekike commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Related PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

HTTP/1 createServer() options to pass custom Request and Response classes#15752
hekike wants to merge 2 commits into
nodejs:masterfrom
hekike:feat/http1-request-response-extend

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