-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib: add an alias at addListener on Server connection socket #27325
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This isn't a complete change. Code around line 748 also needs to be modified to add this line: this.addListener = net.Socket.prototype.addListener;
Personally I would also modify the relevant test (test-http-server-unconsume.js) to account for this change.
59dba46
to
9fd9921
Compare
@apapirovski fixed |
Isn't more gentle and logical to add |
why? all this extend and all behavior should be the same as Lines 269 to 273 in 2161690
|
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.
Thanks! LGTM
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Antonius-S @SimonSchick Hi, did you have any other thought on this ? Otherwise we'll land this. |
@ZYSzys Imo this is not a fix but just a band-aid/work-around, this should be fixed using the new listener event. Also doesn't that break I don't really have any authority here and therefor won't block anything but I don't think this should be merged. |
@SimonSchick |
@SimonSchick |
@himself65 imo that PR is a step back. Just add Otherwise, yes we could use an event for newListener but that’s a lot more work... I dunno. Seems like semantics. Also as far as I can tell the main reason to not use events here would be performance. |
I removed |
@Trott added |
027b976
to
f4d2da6
Compare
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. Could add Fixes: https://github.com/nodejs/node/issues/27199
to the commit message?
This comment has been minimized.
This comment has been minimized.
sure |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
no one to merge this? |
Landed in be26f6e, thanks for the PR! 🎉 (Changed the ommit message changed to begin with |
Fixes: #27199 PR-URL: #27325 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: #27199 PR-URL: #27325 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
about #27199
socket.addListener
should be the same behavior withsocket.on
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes