-
Notifications
You must be signed in to change notification settings - Fork 7.3k
events: fix: shared object references #25467
Conversation
@Hunchmun ... you should be able to squash these commits down easily by doing a |
@jasnell Thanks for your help, completed. |
@@ -44,6 +44,11 @@ calls passing the same combination of `event` and `listener` will result in the | |||
|
|||
Returns emitter, so calls can be chained. | |||
|
|||
**Note:** Due to Pass by Reference, any objects passed to event listeners will be |
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.
How about this?
Note: Objects are passed by reference, meaning that the same object can be passed to multiple listeners. If a listener needs to modify a shared object, a copy of the object should be made.
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 wonder if this note should go on emit()
instead of addListener()
.
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.
Likely. and +1 on the revision.
Updated documentation as per the issue below: #25466 Event listeners can alter parts of the passed object, in some circumstances the changes are passed to the next listeners due to pass by reference. This is documentation of that behavior.
@Hunchmun ... did you mean to close this? |
@jasnell nope, my apologies, this was as a result of resetting the branch, forgot to reopen, Thanks, |
Thanks @Hunchmun! ... LGTM |
Updated documentation as per the issue below: #25466 Event listeners can alter parts of the passed object, in some circumstances the changes are passed to the next listeners due to pass by reference. This is documentation of that behavior. PR-URL: #25467 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Landed in a24db43 |
Updated documentation as per the issue below: nodejs#25466 Event listeners can alter parts of the passed object, in some circumstances the changes are passed to the next listeners due to pass by reference. This is documentation of that behavior. PR-URL: nodejs#25467 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Fixes the issue: #25466
When emitting to multiple listeners for the same event type, the for
loop iteration over these listeners shares the same arguments. This
means (for objects), the copy is only shallow, and changes made by one
listener are overwritten on the object for the next listener.
For example, if we pass an object with arrays: as below
And the first listener sets the users array as null like so:
The second event listener will ALWAYS show the array as null. This is
because only a shallow copy of the arguments is made and not a deep
copy.
This fixes that issue, by enforcing a deep copy and removing all
references from the new arguments array passed to each listener.
Link to code used to reproduce this issue:
https://ghostbin.com/paste/v4ova
Link to issue:
#25466