Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign uphttp2: release request()'s "connect" event listener after it runs #21916
Conversation
targos
approved these changes
Jul 21, 2018
This comment has been minimized.
This comment has been minimized.
|
/cc @nodejs/http2 |
trivikr
approved these changes
Jul 22, 2018
This comment has been minimized.
This comment has been minimized.
trivikr
added
http2
author ready
labels
Jul 22, 2018
addaleax
approved these changes
Jul 22, 2018
apapirovski
approved these changes
Jul 22, 2018
cjihrig
approved these changes
Jul 22, 2018
antsmartian
approved these changes
Jul 23, 2018
jasnell
approved these changes
Jul 23, 2018
mcollina
approved these changes
Jul 23, 2018
|
LGTM, could you add a unit test for this behavior? |
ide
force-pushed the
ide:http2-onconnect
branch
from
69d06f7
to
2e6649c
Jul 28, 2018
This comment has been minimized.
This comment has been minimized.
|
@mcollina Thanks for reviewing. I added a unit test that fails without this commit and passes with it. |
mcollina
approved these changes
Jul 29, 2018
|
LGTM, good work! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Landed in 2aca095 |
targos
closed this
Jul 29, 2018
targos
added a commit
that referenced
this pull request
Jul 29, 2018
targos
added a commit
that referenced
this pull request
Jul 31, 2018
This was referenced Aug 1, 2018
kjin
added a commit
to kjin/node
that referenced
this pull request
Aug 23, 2018
kjin
added a commit
to kjin/node
that referenced
this pull request
Sep 17, 2018
kjin
added a commit
to kjin/node
that referenced
this pull request
Sep 19, 2018
kjin
added a commit
to kjin/node
that referenced
this pull request
Sep 25, 2018
kjin
added a commit
to kjin/node
that referenced
this pull request
Oct 16, 2018
BethGriggs
added a commit
to BethGriggs/node
that referenced
this pull request
Oct 16, 2018
BethGriggs
added a commit
that referenced
this pull request
Oct 17, 2018
beevelop
referenced this pull request
Nov 20, 2018
Closed
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs #250
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
ide commentedJul 21, 2018
The
Http2Session#request()method internally listens to the "connect"event if the session has not yet established a connection so that the
actual request can be sent after the connection has been established.
This commit removes the event listener after it runs and carries out
the request and is no longer needed. In practice this shouldn't affect
the behavior of the session object since the "connect" event fires only
once anyway, but removing the listener releases its references. The
rest of this class subscribes to the "connect" event with
onceinstead of
onas well.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes