-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
@TimothyGu @apapirovski ping |
There was a problem hiding this 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; | ||
} |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done a7c310f
There was a problem hiding this 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.
CI: https://ci.nodejs.org/job/node-test-pull-request/12731/ (the infrastructure should be less flaky now) |
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. |
Landed in 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>
yay! \o/ |
This landed cleanly on v8.x Should this be backported to |
@MylesBorins How to know if this should be backported or not? Just check the code? |
|
@tniessen Shouldnt this part be in backporting-to-release-lines.md? |
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. |
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>
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>
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>
Resolves #18302
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)