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

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

Closed
wants to merge 3 commits into from

Conversation

prog1dev
Copy link
Contributor

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++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 24, 2018
@prog1dev
Copy link
Contributor Author

@TimothyGu @apapirovski ping

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Looks good! One tiny comment.

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

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

Choose a reason for hiding this comment

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

done a7c310f

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM with @TimothyGu's nit.

@apapirovski
Copy link
Member

@apapirovski
Copy link
Member

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
Copy link
Member

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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2018
@BridgeAR
Copy link
Member

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 pull request Feb 1, 2018
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 commented Feb 1, 2018

yay! \o/

@prog1dev prog1dev deleted the fix_18302 branch February 1, 2018 10:33
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
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 pull request Feb 21, 2018
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 pull request Feb 21, 2018
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
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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
Contributor

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 commented Mar 20, 2018

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

@tniessen
Copy link
Member

tniessen commented Mar 20, 2018

@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

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

@TimothyGu
Copy link
Member

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.

MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
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 pull request Mar 30, 2018
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 pull request Apr 1, 2018
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 pull request Apr 13, 2018
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
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URLHost not robust when re-setting its value
9 participants