-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
async_hooks: avoid decrementing iterator after erase #42749
async_hooks: avoid decrementing iterator after erase #42749
Conversation
decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found.
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.
I imagine there's no easy way to add a test for this, but mentioning it anyway in case I'm wrong.
This comment was marked as outdated.
This comment was marked as outdated.
I also don't think that would be easy to write since it is difficult to test undefined behaviour. It might even act differently on different stl implementations (on some even work correctly). But the new approach should not be controversial and existing tests must stay all green. |
This comment was marked as outdated.
This comment was marked as outdated.
Landed in 083d4c4 |
decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found. PR-URL: nodejs#42749 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found. PR-URL: #42749 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found. PR-URL: #42749 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found. PR-URL: #42749 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found. PR-URL: #42749 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found. PR-URL: nodejs/node#42749 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
decrementing an iterator returned by
std::vector::erase
may haveundefined behaviour and should not be used. Decrementing
end()
on an empty container is undefined and
.erase()
could leavethe container empty.
Instead, by calling
vec.erase(it--)
we decrement the valid iteratorbefore the erase operation but after being passed to the erase method.
In case of
AsyncHooks::RemoveContext
perform the cleanup of emptycontexts upfront using
std::remove_if
because the iteration getsinterrupted as soon as the context to be removed has been found.