chore: add missing await to floating promises #5813

yury-s · 2021-03-12T23:14:08Z

We have quite a few places where returned promises have no catch block. Some of them are valid and never expected to throw (this are now marked with void operator) others were missing await or catch block. Fixing some of them to give an idea of what it would look like. If we fix remaining occurrences we can enable no-floating-promises eslint check to prevent this problem in the future.

dgozman

Overall, I think this adds more noise than solves problems. Some are good catches though. I'd advise against enabling the plugin.

src/client/android.ts

dgozman · 2021-03-13T01:56:38Z

src/server/chromium/crConnection.ts

@@ -100,7 +100,7 @@ export class CRConnection extends EventEmitter {
    for (const session of this._sessions.values())
      session._onClosed(browserDisconnectedLogs);
    this._sessions.clear();
-    Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected));
+    void Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected));


I don't quite like these voids for no real reason 😄

For these void Promise.resolve().then( calls we can either introduce a helper function that will do .catch internally or can add .catch() inline which is silly. Which way you like better?

I like neither 😄

dgozman · 2021-03-13T01:57:10Z

src/server/chromium/crNetworkManager.ts

        requestId: event.requestId
      });
    }
    if (!event.networkId) {
      // Fetch without networkId means that request was not recongnized by inspector, and
      // it will never receive Network.requestWillBeSent. Most likely, this is an internal request
      // that we can safely fail.
-      this._client._sendMayFail('Fetch.failRequest', {
+      void this._client._sendMayFail('Fetch.failRequest', {


Perhaps we should make _sendMayFail not return a promise?

There are methods like InterceptableRequest.continue that wait on the returned promise.

Ok, updated the code.

dgozman · 2021-03-13T01:58:03Z

src/server/chromium/crPage.ts

@@ -646,7 +646,7 @@ class FrameSession {
    session.once('Runtime.executionContextCreated', async event => {
      worker._createExecutionContext(new CRExecutionContext(session, event.context));
    });
-    Promise.all([
+    void Promise.all([


Can just drop Promise.all.

Yes, but it would be even more verbose as it'd require void prefix for each statement.

Removed Promise.all and made replaced the methods with synchronous version.

dgozman · 2021-03-13T01:58:59Z

src/server/chromium/crPage.ts

@@ -969,7 +969,7 @@ class FrameSession {
    });
    const frameId = nodeInfo && typeof nodeInfo.node.frameId === 'string' ?
      nodeInfo.node.frameId : null;
-    documentElement.dispose();
+    void documentElement.dispose();


Same here, we can make handle.dispose() not return a Promise.

Overall, I think this adds more noise than solves problems. Some are good catches though. I'd advise against enabling the plugin.

Yeah, I'm not fond of this void prefixes all over the code. Without them there is a lot of noise in the linter output masking real issues but we can live with that for now.

Changed dispose() to not return promise.

dgozman · 2021-03-16T15:14:32Z

src/server/chromium/crConnection.ts

@@ -100,7 +100,7 @@ export class CRConnection extends EventEmitter {
    for (const session of this._sessions.values())
      session._onClosed(browserDisconnectedLogs);
    this._sessions.clear();
-    Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected));
+    void Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected));


I like neither 😄

dgozman · 2021-03-16T15:16:15Z

src/server/firefox/ffExecutionContext.ts

@@ -90,7 +90,7 @@ export class FFExecutionContext implements js.ExecutionContextDelegate {
    await this._session.send('Runtime.disposeObject', {
      executionContextId: this._executionContextId,
      objectId: handle._objectId,
-    }).catch(error => {});
+    });


Why this change?

It's caller's responsibility to catch exceptions if any and it does so now in .dispose().

dgozman · 2021-03-16T15:17:36Z

src/server/input.ts

@@ -210,7 +210,7 @@ export class Mouse {
  async click(x: number, y: number, options: { delay?: number, button?: types.MouseButton, clickCount?: number } = {}) {
    const { delay = null, clickCount = 1 } = options;
    if (delay) {
-      this.move(x, y);
+      await this.move(x, y);


This was not awaiting on purpose.

Sounds sketchy and should be documented in that case.

src/server/snapshot/inMemorySnapshotter.ts

dgozman · 2021-03-16T15:20:37Z

src/server/snapshot/snapshotter.ts

+  _annotateFrameHierarchyAsync(frame).catch(e => {});
+}
+
+async function _annotateFrameHierarchyAsync(frame: Frame) {


Reminds me of C# 😄

dgozman · 2021-03-16T15:21:26Z

src/server/supplements/inspectorController.ts

@@ -45,15 +45,15 @@ export class InspectorController implements InstrumentationListener {
        case 'log':
          const originalMetadata = this._waitOperations.get(info.waitId)!;
          originalMetadata.log.push(info.message);
-          this.onCallLog('api', info.message, sdkObject, originalMetadata);
+          await this.onCallLog('api', info.message, sdkObject, originalMetadata);


I think these were without await on purpose.

should be .catch then?

dgozman · 2021-03-16T15:22:46Z

src/server/supplements/recorderSupplement.ts

    }
  }

  async onBeforeCall(sdkObject: SdkObject, metadata: CallMetadata): Promise<void> {
    if (this._mode === 'recording')
      return;
-    this._captureSnapshot(sdkObject, metadata, 'before');
+    await this._captureSnapshot(sdkObject, metadata, 'before');


It is important to no await here.

hm, this should be commented then, maybe .catch too?

dgozman · 2021-03-16T15:23:35Z

src/server/webkit/wkPage.ts

@@ -113,7 +113,7 @@ export class WKPage implements PageDelegate {
    promises.push(this.updateHttpCredentials());
    if (this._browserContext._permissions.size) {
      for (const [key, value] of this._browserContext._permissions)
-        this._grantPermissions(key, value);
+        promises.push(this._grantPermissions(key, value));


Good catch!

yury-s · 2021-03-20T00:54:15Z

Removed controversial parts, left only simple ones.

dgozman

Looks great!

yury-s force-pushed the no-floating-promises-1 branch from 45546e0 to 6bd870a Compare March 12, 2021 23:15

dgozman reviewed Mar 13, 2021

View reviewed changes

dgozman reviewed Mar 16, 2021

View reviewed changes

yury-s added 2 commits March 19, 2021 17:19

chore: add missing await annotate floating promises

1cd9756

Reverted some of the changes

c0c885d

yury-s force-pushed the no-floating-promises-1 branch from 3026c51 to c0c885d Compare March 20, 2021 00:51

yury-s changed the title ~~chore: add missing await annotate floating promises (1)~~ chore: add missing await to floating promises Mar 20, 2021

yury-s added 3 commits March 19, 2021 18:19

Revert inMemorySnapshotter.ts

cfbc87e

Revert linter config

40e28c0

Revert .only

b2cea17

dgozman approved these changes Mar 22, 2021

View reviewed changes

yury-s merged commit 67c29e8 into microsoft:master Mar 22, 2021

yury-s deleted the no-floating-promises-1 branch March 22, 2021 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add missing await to floating promises #5813

chore: add missing await to floating promises #5813

yury-s commented Mar 12, 2021 •

edited

Loading

dgozman left a comment

dgozman Mar 13, 2021

yury-s Mar 15, 2021

dgozman Mar 16, 2021

dgozman Mar 13, 2021

yury-s Mar 15, 2021

yury-s Mar 15, 2021

dgozman Mar 13, 2021

yury-s Mar 15, 2021

yury-s Mar 15, 2021

dgozman Mar 13, 2021

yury-s Mar 15, 2021

yury-s Mar 15, 2021

dgozman Mar 16, 2021

dgozman Mar 16, 2021

yury-s Mar 16, 2021

dgozman Mar 16, 2021

yury-s Mar 16, 2021 •

edited

Loading

dgozman Mar 16, 2021

dgozman Mar 16, 2021

yury-s Mar 16, 2021

dgozman Mar 16, 2021

yury-s Mar 16, 2021

dgozman Mar 16, 2021

yury-s commented Mar 20, 2021

dgozman left a comment

chore: add missing await to floating promises #5813

chore: add missing await to floating promises #5813

Conversation

yury-s commented Mar 12, 2021 • edited Loading

dgozman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yury-s Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yury-s commented Mar 20, 2021

dgozman left a comment

Choose a reason for hiding this comment

yury-s commented Mar 12, 2021 •

edited

Loading

yury-s Mar 16, 2021 •

edited

Loading