Skip to content
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

chore: less async await #2463

Merged
merged 2 commits into from
Nov 27, 2023
Merged

chore: less async await #2463

merged 2 commits into from
Nov 27, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 27, 2023

Why do we use so much async await?

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2023

Codecov Report

Attention: 62 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (2b49bf6) 85.62%.
Report is 107 commits behind head on main.

Files Patch % Lines
lib/handler/RetryHandler.js 73.04% 31 Missing ⚠️
lib/fetch/index.js 66.66% 11 Missing ⚠️
lib/api/readable.js 73.91% 6 Missing ⚠️
lib/fetch/headers.js 87.80% 5 Missing ⚠️
lib/client.js 92.10% 3 Missing ⚠️
lib/core/util.js 90.47% 2 Missing ⚠️
lib/fetch/request.js 86.66% 2 Missing ⚠️
lib/core/request.js 97.56% 1 Missing ⚠️
lib/proxy-agent.js 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2463      +/-   ##
==========================================
+ Coverage   85.54%   85.62%   +0.07%     
==========================================
  Files          76       77       +1     
  Lines        6858     7053     +195     
==========================================
+ Hits         5867     6039     +172     
- Misses        991     1014      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit e7ab79a into nodejs:main Nov 27, 2023
17 of 18 checks passed
await this[kAgent].close()
await this[kClient].close()
[kClose] () {
return Promise.all([this[kAgent].close, this[kClient].close])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Uzlopak @mcollina @KhafraDev ?:

return Promise.all([this[kAgent].close(), this[kClient].close()])

And 👇🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I searched for [kClose] and the places were it was was not awaited anyway.

@@ -1808,10 +1808,10 @@ async function httpNetworkFetch (
fetchParams.controller.controller = controller
},
async pull (controller) {
await pullAlgorithm(controller)
pullAlgorithm(controller)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!! This is wrong? needs a return at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was before not returning anything. So it is fire and forget? So why await anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't it at least informing the caller that the operation is done? I'm not sure where this is used, this can't possible cause racing conditions or break someone's tests?

@@ -82,40 +82,38 @@ class ProxyAgent extends DispatcherBase {
this[kClient] = clientFactory(resolvedUrl, { connect })
this[kAgent] = new Agent({
...opts,
connect: async (opts, callback) => {
connect: (opts, callback) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was unnecessary and made it less maintainable...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was also questionable for me. But it should save a Promise?

@ronag
Copy link
Member

ronag commented Nov 27, 2023

@mcollina @Uzlopak I would recommend reverting this, it doesn't improve things significantly and it introduces some problems...

@ronag
Copy link
Member

ronag commented Nov 27, 2023

@KhafraDev also ☝️

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 27, 2023

First of all, I expected a little bit more discussion before merging.
Secondly, I expect that for a reason that we have alot of Promises for a fetch call, is the unecessary wrapping of Promises in async functions.And awaiting Promises in internally used async functions.

As the tests pass, I was like: "Ok, seems to be right" and was hoping to get some thoughts, and maybe revert some parts but still get some changes through.

mcollina added a commit that referenced this pull request Nov 27, 2023
@mcollina
Copy link
Member

Done, sorry

@mcollina
Copy link
Member

Can you reopen @Uzlopak ?

@Uzlopak Uzlopak mentioned this pull request Nov 27, 2023
7 tasks
@giacomorebonato
Copy link

I thought that the point of return await was to have cleaner stacktraces about where the error resides.
Also seeing a function marked as async is a clear statement about something, while omitting async removes this clarity.

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants