★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18357Nouvelle recherche | Portail wanayoo
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: free up memory before re-setting URLHost value #18357

Closed
wants to merge 3 commits into from
Closed

src: free up memory before re-setting URLHost value #18357

wants to merge 3 commits into from

Conversation

prog1dev
Copy link
Contributor

@prog1dev prog1dev commented Jan 24, 2018

Resolves #18302

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • Version: master
  • Platform: all
  • Subsystem: url, src

@nodejs-github-bot nodejs-github-bot added c++ dont-land-on-v4.x whatwg-url labels Jan 24, 2018
@prog1dev
Copy link
Contributor Author

@prog1dev prog1dev commented Jan 24, 2018

@TimothyGu @apapirovski ping

TimothyGu
TimothyGu approved these changes Jan 24, 2018
View changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Looks good! One tiny comment.

src/node_url.cc
case HostType::H_DOMAIN: value_.domain.~string(); break;
case HostType::H_OPAQUE: value_.opaque.~string(); break;
default: break;
}
Copy link
Member

@TimothyGu TimothyGu Jan 24, 2018

Choose a reason for hiding this comment

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

After resettig the value_, can you reset the type_ as well by setting it to HostType::H_FAILED?

Copy link
Contributor Author

@prog1dev prog1dev Jan 24, 2018 •
edited

Choose a reason for hiding this comment

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

done a7c310f

apapirovski
apapirovski approved these changes Jan 24, 2018
View changes
Copy link
Member

@apapirovski apapirovski left a comment

LGTM with @TimothyGu's nit.

@apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 24, 2018

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

@apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 25, 2018

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

(the infrastructure should be less flaky now)

@prog1dev prog1dev changed the title Fix 18302 src: free up memory before re-setting URLHost value Jan 25, 2018
@TimothyGu TimothyGu requested a review from addaleax Jan 26, 2018
@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jan 26, 2018

The Windows issues look unrelated, as the Windows CI jobs have been unwell for a while now: https://ci.nodejs.org/job/node-test-commit-windows-fanned/

Unpinning dont-land-on-v6.x, as the URL parser will go into v6.x soon.

@TimothyGu TimothyGu added the author ready label Jan 26, 2018
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 1, 2018

Landed in 36fd25f

@BridgeAR BridgeAR closed this Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Feb 1, 2018
@prog1dev @BridgeAR
src: free memory before re-setting URLHost value
36fd25f 
Fixes: nodejs#18302

PR-URL: nodejs#18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@prog1dev
Copy link
Contributor Author

@prog1dev prog1dev commented Feb 1, 2018

yay! \o/

@prog1dev prog1dev deleted the fix_18302 branch Feb 1, 2018
@addaleax addaleax removed the author ready label Feb 4, 2018
MylesBorins pushed a commit that referenced this issue Feb 20, 2018
@prog1dev @MylesBorins
src: free memory before re-setting URLHost value
6f2c51c 
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
@prog1dev @MylesBorins
src: free memory before re-setting URLHost value
874361d 
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
@prog1dev @MylesBorins
src: free memory before re-setting URLHost value
43956e9 
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
@prog1dev @MylesBorins
src: free memory before re-setting URLHost value
265faf8 
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Mar 20, 2018

This landed cleanly on v8.x

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@prog1dev
Copy link
Contributor Author

@prog1dev prog1dev commented Mar 20, 2018 •
edited

@MylesBorins How to know if this should be backported or not? Just check the code?

@tniessen
Copy link
Member

@tniessen tniessen commented Mar 20, 2018 •
edited

@prog1dev

  1. Will it cause problems on that branch or incompatibilities with our or third-party code, including JS modules, or is it marked as semver-major? → Don't backport.
  2. Will people using v6.x benefit from this change? → Backport.
  3. Will it make backporting other changes easier? → Backport.
  4. None of the above? → Don't backport.

@prog1dev
Copy link
Contributor Author

@prog1dev prog1dev commented Mar 20, 2018

@tniessen Shouldnt this part be in backporting-to-release-lines.md?

@prog1dev prog1dev mentioned this pull request Mar 21, 2018
Closed
@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Mar 23, 2018

Yes, this should indeed be backported to v6.x. It doesn't fix any known bugs, but increases the robustness of the code in question.

@prog1dev prog1dev mentioned this pull request Mar 27, 2018
Closed
2 tasks
MylesBorins pushed a commit that referenced this issue Mar 28, 2018
@prog1dev @MylesBorins
src: free memory before re-setting URLHost value
06a28aa 
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Mar 30, 2018
@prog1dev @MylesBorins
src: free memory before re-setting URLHost value
c0f40be 
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
prog1dev added a commit to prog1dev/node that referenced this issue Apr 1, 2018
@prog1dev
src: free memory before re-setting URLHost value
af2c41e 
Fixes: nodejs#18302

PR-URL: nodejs#18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Apr 13, 2018
@prog1dev @MylesBorins
src: free memory before re-setting URLHost value
0ca2dad 
Fixes: #18302

Backport-PR-URL: #19639
PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Apr 13, 2018
Merged
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
@prog1dev @MayaLekova
src: free memory before re-setting URLHost value
23e845c 
Fixes: nodejs#18302

PR-URL: nodejs#18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski

@jasnell jasnell

@addaleax addaleax

@TimothyGu TimothyGu

@tniessen tniessen

Assignees
No one assigned
Labels
c++ whatwg-url
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

URLHost not robust when re-setting its value
9 participants
@prog1dev @apapirovski @TimothyGu @BridgeAR @MylesBorins @tniessen @jasnell @addaleax @nodejs-github-bot