unnecessary async?

[Uzlopak]

Is there a reason we have some async functions which dont need to be async?

diff --git a/lib/api/readable.js b/lib/api/readable.js
index 4e440b21..9d085687 100644
--- a/lib/api/readable.js
+++ b/lib/api/readable.js
@@ -164,7 +164,7 @@ class BodyReadable extends Readable {
    * @see https://fetch.spec.whatwg.org/#dom-body-text
    * @returns {Promise<string>}
    */
-  async text () {
+  text () {
     return consume(this, 'text')
   }
 
@@ -174,7 +174,7 @@ class BodyReadable extends Readable {
    * @see https://fetch.spec.whatwg.org/#dom-body-json
    * @returns {Promise<unknown>}
    */
-  async json () {
+  json () {
     return consume(this, 'json')
   }
 
@@ -184,7 +184,7 @@ class BodyReadable extends Readable {
    * @see https://fetch.spec.whatwg.org/#dom-body-blob
    * @returns {Promise<Blob>}
    */
-  async blob () {
+  blob () {
     return consume(this, 'blob')
   }
 
@@ -194,7 +194,7 @@ class BodyReadable extends Readable {
    * @see https://fetch.spec.whatwg.org/#dom-body-bytes
    * @returns {Promise<Uint8Array>}
    */
-  async bytes () {
+  bytes () {
     return consume(this, 'bytes')
   }
 
@@ -204,7 +204,7 @@ class BodyReadable extends Readable {
    * @see https://fetch.spec.whatwg.org/#dom-body-arraybuffer
    * @returns {Promise<ArrayBuffer>}
    */
-  async arrayBuffer () {
+  arrayBuffer () {
     return consume(this, 'arrayBuffer')
   }
 
@@ -214,7 +214,7 @@ class BodyReadable extends Readable {
    * @see https://fetch.spec.whatwg.org/#dom-body-formdata
    * @throws {NotSupportedError}
    */
-  async formData () {
+  formData () {
     // TODO: Implement.
     throw new NotSupportedError()
   }
@@ -255,7 +255,7 @@ class BodyReadable extends Readable {
    * @param {AbortSignal} [opts.signal] An AbortSignal to cancel the dump.
    * @returns {Promise<null>}
    */
-  async dump (opts) {
+  dump (opts) {
     const signal = opts?.signal
 
     if (signal != null && (typeof signal !== 'object' || !('aborted' in signal))) {
@@ -272,7 +272,7 @@ class BodyReadable extends Readable {
       return null
     }
 
-    return await new Promise((resolve, reject) => {
+    return new Promise((resolve, reject) => {
       if (
         (this[kContentLength] && (this[kContentLength] > limit)) ||
         this[kBytesRead] > limit
@@ -355,7 +355,7 @@ function isUnusable (bodyReadable) {
  * @param {string} type
  * @returns {Promise<any>}
  */
-async function consume (stream, type) {
+function consume (stream, type) {
   assert(!stream[kConsume])
 
   return new Promise((resolve, reject) => {
diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js
index 46fc1574..24a87860 100644
--- a/lib/dispatcher/agent.js
+++ b/lib/dispatcher/agent.js
@@ -14,6 +14,8 @@ const kOnDrain = Symbol('onDrain')
 const kFactory = Symbol('factory')
 const kOptions = Symbol('options')
 
+function noop () {}
+
 function defaultFactory (origin, opts) {
   return opts && opts.connections === 1
     ? new Client(origin, opts)
@@ -91,24 +93,24 @@ class Agent extends DispatcherBase {
     return dispatcher.dispatch(opts, handler)
   }
 
-  async [kClose] () {
+  [kClose] () {
     const closePromises = []
     for (const client of this[kClients].values()) {
       closePromises.push(client.close())
     }
     this[kClients].clear()
 
-    await Promise.all(closePromises)
+    return Promise.all(closePromises).then(noop)
   }
 
-  async [kDestroy] (err) {
+  [kDestroy] (err) {
     const destroyPromises = []
     for (const client of this[kClients].values()) {
       destroyPromises.push(client.destroy(err))
     }
     this[kClients].clear()
 
-    await Promise.all(destroyPromises)
+    return Promise.all(destroyPromises).then(noop)
   }
 }
 
diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js
index 65380f19..6965ec56 100644
--- a/lib/dispatcher/client.js
+++ b/lib/dispatcher/client.js
@@ -301,7 +301,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) => {
@@ -313,7 +313,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++) {

[metcoder95]

For those that does an explicit await, I'd like to preserve it for debugging purposes as it will contiant he stacktrace of the method that called, which can help assess bug reports.

For others, I'm not 100% sure

[mcollina]

Stacktraces.

[Uzlopak]

@mcollina

Can you please elaborate? I mean, we dont await most of the "wrapped" promises.

[mcollina]

If a promise is awaited or returned by an async function, that async function will show up in stack traces. Those are publicly visible functions, so it might make sense to keep.

[Uzlopak]

We dont have TCO in v8. So shouldnt we still see the stack trace properly?

[mcollina]

No it wouldn't. V8 has async stack trace support.

[Uzlopak]

@mcollina

do you see any performance benenefit by avoiding async notation in cases where we dont await the promise?

[mcollina]

Yes, there is a performance benefit.

As for when skipping it... It really depends on the place. If it's an internal function that is very hot, I'd recommend skipping. If it's a public function, then it's better to keep it.