-
-
Notifications
You must be signed in to change notification settings - Fork 660
remove creating an extra Promise just for common cleanup #4339
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
Conversation
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.
changes are not matching description
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
@Uzlopak what doesn't match? |
I would basically expect an added try catch block, everything inside being just white space changes, because of the intendation changes, but here also comments are changed and behaviour of fetch seems also be modified. |
Here are 2 dumps of async hooks logs that show the extra allocation and Based upon Node 24.4.1:
|
@Uzlopak there are cases where that Here is a screenshot of the github UI showing no comment changes. |
Tests are unfortunately failing. |
This comment was marked as outdated.
This comment was marked as outdated.
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.
github doesn't render the changes very well but I believe I'm making sense of it. all looks good to me to avoid the extra promise
fix tests
OK, i had a look at it at the hotel and i modified it to pass the tests. |
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
@Uzlopak can you approve then? |
When i pushed I revoked my review as i modified the code myself. I approve if you like, no problem. The merge button is not green because of a failing test in windows. I checked but it doesnt show why. |
Ah! It passed now. @bmeck Also i thank you personally. That |
This relates to...
N/A
Rationale
While debugging some Promise allocation and lack of use I found extra ticks associated with a
.catch
in undici that are creating a Promise, adopting the result of another promise and then creating 1 extra tick.Changes
I made a change that removes a closure and extra Promise allocation around fetching => mainFetch. This doesn't fully remove wast as mainFetch is still performing an extra allocation and tick at end of the function that isn't utilized.
I didn't see a direct spec correspondance with the controller being terminated that was the cause of using
.catch
but left the behavior unchanged except doing it on an earlier tick.Features
Bug Fixes
Breaking Changes and Deprecations
Status