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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class Agent extends DispatcherBase {
return dispatcher.dispatch(opts, handler)
}

async [kClose] () {
[kClose] () {
const closePromises = []
for (const ref of this[kClients].values()) {
const client = ref.deref()
Expand All @@ -128,10 +128,10 @@ class Agent extends DispatcherBase {
}
}

await Promise.all(closePromises)
return Promise.all(closePromises)
}

async [kDestroy] (err) {
[kDestroy] (err) {
const destroyPromises = []
for (const ref of this[kClients].values()) {
const client = ref.deref()
Expand All @@ -141,7 +141,7 @@ class Agent extends DispatcherBase {
}
}

await Promise.all(destroyPromises)
return Promise.all(destroyPromises)
}
}

Expand Down
8 changes: 5 additions & 3 deletions lib/api/readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,14 @@ function isUnusable (self) {
return util.isDisturbed(self) || isLocked(self)
}

async function consume (stream, type) {
function consume (stream, type) {
if (isUnusable(stream)) {
throw new TypeError('unusable')
return Promise.reject(new TypeError('unusable'))
}

assert(!stream[kConsume])
if (stream[kConsume]) {
return Promise.reject(new assert.AssertionError('null != true'))
}

return new Promise((resolve, reject) => {
stream[kConsume] = {
Expand Down
4 changes: 2 additions & 2 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ class Client extends DispatcherBase {
return this[kNeedDrain] < 2
}

async [kClose] () {
[kClose] () {
// TODO: for H2 we need to gracefully flush the remaining enqueued
// request and close each stream.
return new Promise((resolve) => {
Expand All @@ -392,7 +392,7 @@ class Client extends DispatcherBase {
})
}

async [kDestroy] (err) {
[kDestroy] (err) {
return new Promise((resolve) => {
const requests = this[kQueue].splice(this[kPendingIdx])
for (let i = 0; i < requests.length; i++) {
Expand Down
4 changes: 2 additions & 2 deletions lib/fetch/body.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ function extractBody (object, keepalive = false) {
}
return controller.desiredSize > 0
},
async cancel (reason) {
await iterator.return()
cancel (reason) {
return iterator.return()
},
type: undefined
})
Expand Down
4 changes: 2 additions & 2 deletions lib/fetch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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?

},
async cancel (reason) {
await cancelAlgorithm(reason)
cancelAlgorithm(reason)
}
},
{
Expand Down
4 changes: 2 additions & 2 deletions lib/pool-base.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class PoolBase extends DispatcherBase {
return this[kStats]
}

async [kClose] () {
[kClose] () {
if (this[kQueue].isEmpty()) {
return Promise.all(this[kClients].map(c => c.close()))
} else {
Expand All @@ -121,7 +121,7 @@ class PoolBase extends DispatcherBase {
}
}

async [kDestroy] (err) {
[kDestroy] (err) {
while (true) {
const item = this[kQueue].shift()
if (!item) {
Expand Down
66 changes: 31 additions & 35 deletions lib/proxy-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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?

let requestedHost = opts.host
if (!opts.port) {
requestedHost += `:${defaultProtocolPort(opts.protocol)}`
}
try {
const { socket, statusCode } = await this[kClient].connect({
origin,
port,
path: requestedHost,
signal: opts.signal,
headers: {
...this[kProxyHeaders],
host
}
})
if (statusCode !== 200) {
socket.on('error', () => {}).destroy()
callback(new RequestAbortedError('Proxy response !== 200 when HTTP Tunneling'))
}
if (opts.protocol !== 'https:') {
callback(null, socket)
return
this[kClient].connect({
origin,
port,
path: requestedHost,
signal: opts.signal,
headers: {
...this[kProxyHeaders],
host
}
let servername
if (this[kRequestTls]) {
servername = this[kRequestTls].servername
} else {
servername = opts.servername
}
this[kConnectEndpoint]({ ...opts, servername, httpSocket: socket }, callback)
} catch (err) {
callback(err)
}
}).catch(callback)
.then(({ socket, statusCode }) => {
if (statusCode !== 200) {
socket.on('error', () => { }).destroy()
callback(new RequestAbortedError('Proxy response !== 200 when HTTP Tunneling'))
}
if (opts.protocol !== 'https:') {
callback(null, socket)
return
}
let servername
if (this[kRequestTls]) {
servername = this[kRequestTls].servername
} else {
servername = opts.servername
}
this[kConnectEndpoint]({ ...opts, servername, httpSocket: socket }, callback)
}).catch(callback)
}
})
}
Expand All @@ -136,14 +134,12 @@ class ProxyAgent extends DispatcherBase {
)
}

async [kClose] () {
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.

}

async [kDestroy] () {
await this[kAgent].destroy()
await this[kClient].destroy()
[kDestroy] () {
return Promise.all([this[kAgent].destroy, this[kClient].destroy])
}
}

Expand Down