http2: multiple cleanups and s/streamClosed/close #17328

jasnell · 2017-11-26T18:45:26Z

Includes multiple cleanups in core.js along with:

Use correct connect event for TLSSocket
Use 'close' event instead of streamClosed

/cc @nodejs/http2

Fixes: #15303

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)

http2

jasnell · 2017-11-26T18:46:11Z

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

addaleax · 2017-11-26T18:58:25Z

lib/internal/http2/core.js

-      socket.once('connect', setupFn);
+      const connectEvent =
+        socket instanceof tls.TLSSocket ? 'secureConnect' : 'connect';
+      socket.once(connectEvent, setupFn);


Is it possible to add a test for this?

There are already tests for it that have been flaky, this should fix those flaky tests. One of the existing ones could be modified to check that a secureConnect handler has been registered.

@jasnell Can you point me to one? I volunteer to try and make it fail/pass consistently :)

http2-create-client-secure-session is one, which should be fixed by this PR.
I've updated it to include the check for secureConnect

apapirovski

LGTM 👍

apapirovski · 2017-11-26T18:57:12Z

lib/internal/http2/compat.js

@@ -250,7 +250,7 @@ class Http2ServerRequest extends Readable {
    stream.on('close', onStreamClosedRequest);


Can you remove this and the associated function? They should not be necessary.

apapirovski · 2017-11-26T18:57:26Z

lib/internal/http2/compat.js

@@ -383,7 +383,7 @@ class Http2ServerResponse extends Stream {
    stream.on('close', onStreamClosedResponse);


Same as above.

Can you remove this and the associated function? They should not be necessary.

apapirovski · 2017-11-26T19:01:35Z

lib/internal/http2/core.js

@@ -959,13 +968,14 @@ class Http2Session extends EventEmitter {
      this.on('shutdown', callback);
    }

+    const fn = submitShutdown.bind(this, options);


Could this be a bit more descriptively named? (Similar to setupFn elsewhere in this code.)

jasnell · 2017-11-27T04:10:07Z

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

sebdeckers

LGTM 👍 This event confused me many times. Thanks for clearing it up.

General improvements to test and verify that a secureConnect handler is present

jasnell · 2017-11-28T17:34:27Z

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

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

General improvements to test and verify that a secureConnect handler is present PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

jasnell · 2017-11-28T18:38:02Z

Landed in 0fb1e07...093a870

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

General improvements to test and verify that a secureConnect handler is present PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

General improvements to test and verify that a secureConnect handler is present PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 26, 2017

jasnell added the http2 Issues or PRs related to the http2 subsystem. label Nov 26, 2017

addaleax reviewed Nov 26, 2017

View reviewed changes

apapirovski approved these changes Nov 26, 2017

View reviewed changes

sebdeckers approved these changes Nov 27, 2017

View reviewed changes

jasnell added 7 commits November 27, 2017 10:35

http2: use correct connect event for TLS Socket

516881e

http2: general cleanups

a21f2c9

http2: reduce code duplication in settings

48514ce

http2: use 'close' event instead of 'streamClosed'

e7ad83d

test: fixup test-http2-create-client-secure-session

72c5f1f

General improvements to test and verify that a secureConnect handler is present

http2: remove unnecessary event handlers

379f999

http2: use more descriptive names

4b964e5

jasnell force-pushed the http2-refactor4 branch from 2e5bcdb to 4b964e5 Compare November 27, 2017 19:43

jasnell added a commit that referenced this pull request Nov 28, 2017

http2: use correct connect event for TLS Socket

f3686f2

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

jasnell added a commit that referenced this pull request Nov 28, 2017

http2: general cleanups

d4111d0

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

jasnell added a commit that referenced this pull request Nov 28, 2017

http2: reduce code duplication in settings

df33d8d

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

jasnell added a commit that referenced this pull request Nov 28, 2017

http2: use 'close' event instead of 'streamClosed'

1b99542

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

jasnell added a commit that referenced this pull request Nov 28, 2017

http2: remove unnecessary event handlers

973b8e0

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

jasnell added a commit that referenced this pull request Nov 28, 2017

http2: use more descriptive names

093a870

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

jasnell closed this Nov 28, 2017

MylesBorins pushed a commit that referenced this pull request Dec 7, 2017

http2: use correct connect event for TLS Socket

d607718

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 7, 2017

http2: use correct connect event for TLS Socket

c81e968

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

evanlucas pushed a commit that referenced this pull request Dec 7, 2017

http2: use correct connect event for TLS Socket

71cf21a

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

evanlucas mentioned this pull request Dec 7, 2017

v9.2.1 release proposal #17531

Merged

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

http2: general cleanups

e5f92cd

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

http2: reduce code duplication in settings

67abc1e

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

http2: remove unnecessary event handlers

4994d57

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

http2: use more descriptive names

658338e

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins added dont-land-on-v6.x labels Dec 19, 2017

MylesBorins pushed a commit that referenced this pull request Dec 19, 2017

http2: general cleanups

bdcbe81

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 19, 2017

http2: reduce code duplication in settings

31cdbd6

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 19, 2017

http2: remove unnecessary event handlers

9fd0b32

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 19, 2017

http2: use more descriptive names

673e263

PR-URL: #17328 Fixes: #15303 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

This was referenced Dec 20, 2017

v8.9.4 proposal #17772

Closed

v8.9.4 proposal #17774

Merged

kjin mentioned this pull request Dec 21, 2017

grpc-js-core: prevent callback before 'end' event and handle eos headers as trailers grpc/grpc-node#132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: multiple cleanups and s/streamClosed/close #17328

http2: multiple cleanups and s/streamClosed/close #17328

jasnell commented Nov 26, 2017 •

edited

Loading

jasnell commented Nov 26, 2017

addaleax Nov 26, 2017

jasnell Nov 26, 2017

addaleax Nov 26, 2017

jasnell Nov 26, 2017

apapirovski left a comment

apapirovski Nov 26, 2017

apapirovski Nov 26, 2017 •

edited

Loading

apapirovski Nov 26, 2017

jasnell commented Nov 27, 2017

sebdeckers left a comment

jasnell commented Nov 28, 2017

jasnell commented Nov 28, 2017

		@@ -250,7 +250,7 @@ class Http2ServerRequest extends Readable {
		stream.on('close', onStreamClosedRequest);

		@@ -383,7 +383,7 @@ class Http2ServerResponse extends Stream {
		stream.on('close', onStreamClosedResponse);

http2: multiple cleanups and s/streamClosed/close #17328

http2: multiple cleanups and s/streamClosed/close #17328

Conversation

jasnell commented Nov 26, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jasnell commented Nov 26, 2017

addaleax Nov 26, 2017

Choose a reason for hiding this comment

jasnell Nov 26, 2017

Choose a reason for hiding this comment

addaleax Nov 26, 2017

Choose a reason for hiding this comment

jasnell Nov 26, 2017

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Nov 26, 2017

Choose a reason for hiding this comment

apapirovski Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski Nov 26, 2017

Choose a reason for hiding this comment

jasnell commented Nov 27, 2017

sebdeckers left a comment

Choose a reason for hiding this comment

jasnell commented Nov 28, 2017

jasnell commented Nov 28, 2017

jasnell commented Nov 26, 2017 •

edited

Loading

apapirovski Nov 26, 2017 •

edited

Loading