Skip to content

http: free the parser before emitting 'upgrade' - #18209

Closed
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:free/parser-asap
Closed

http: free the parser before emitting 'upgrade'#18209
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:free/parser-asap

Conversation

@lpinca

@lpincalpinca commented Jan 17, 2018•
edited
Loading

Copy link
Copy Markdown
Member

The reason for this change is mainly to be consistent with the server but also to prevent an error from being thrown in the unlikely case where the user assigns something to the parser property when the 'upgrade' event is emitted.

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

@lpinca
http: free the parser before emitting 'upgrade'
4be1e71
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
@nodejs-github-botnodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jan 17, 2018
@maclover7

maclover7 commented Jan 18, 2018

Copy link
Copy Markdown
Contributor

Would you be able to add a test case for the server-freeing-the-parser behavior as well? Otherwise LGTM

@maclover7

maclover7 commented Jan 18, 2018

Copy link
Copy Markdown
Contributor

cc @nodejs/http

@lpinca

lpinca commented Jan 18, 2018

Copy link
Copy Markdown
MemberAuthor

@maclover7 the included test does that.

@addaleax

addaleax commented Jan 18, 2018

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-commit/15503/

indutny
indutny approved these changes Jan 18, 2018
View reviewed changes

@indutnyindutny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good catch.

@maclover7maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 21, 2018
@lpinca@BridgeAR
http: free the parser before emitting 'upgrade'
93f1d9e
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: nodejs#18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR

BridgeAR commented Jan 21, 2018

Copy link
Copy Markdown
Member

Landed in 93f1d9e

@lpinca
lpinca deleted the free/parser-asap branch January 24, 2018 15:45
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@lpinca@evanlucas
http: free the parser before emitting 'upgrade'
7bebb0d
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@evanlucasevanlucas mentioned this pull request Jan 30, 2018
Merged
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@lpinca@evanlucas
http: free the parser before emitting 'upgrade'
0250e1b
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins

MylesBorins commented Feb 27, 2018

Copy link
Copy Markdown
Contributor

should this land on v6.x or v8.x? It lands cleanly for both

@lpinca

lpinca commented Feb 27, 2018

Copy link
Copy Markdown
MemberAuthor

@MylesBorins yes it should imho.

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@lpinca@MylesBorins
http: free the parser before emitting 'upgrade'
fcb1bbe
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@lpinca@MylesBorins
http: free the parser before emitting 'upgrade'
7176e47
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@lpinca@MylesBorins
http: free the parser before emitting 'upgrade'
94a954a
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@lpinca@MylesBorins
http: free the parser before emitting 'upgrade'
f0f7c2b
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@lpinca@MylesBorins
http: free the parser before emitting 'upgrade'
dbe70b7
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@lpinca@MylesBorins
http: free the parser before emitting 'upgrade'
1d88266
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: #18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Apr 13, 2018
Merged
@MylesBorinsMylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@lpinca@MayaLekova
http: free the parser before emitting 'upgrade'
da0788d
Ensure that the parser is freed before emitting the 'connect' or
'upgrade' event.
PR-URL: nodejs#18209
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+2 more reviewers
@indutnyindutnyindutny approved these changes
@maclover7maclover7maclover7 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.httpIssues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@lpinca@maclover7@addaleax@BridgeAR@MylesBorins@indutny@jasnell@nodejs-github-bot