Re-enable `discarded_futures` lint by elliette · Pull Request #9117 · flutter/devtools

elliette · 2025-04-07T20:24:58Z

Follow-up to #9100 which disabled the discarded_futures lint.

kenzieschmoll · 2025-04-07T21:04:24Z

packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart

+      addAutoDisposeListener(_supportsToggleSelectWidgetMode, () async {
        if (_supportsToggleSelectWidgetMode.value) {
-          serviceConnection.serviceManager.serviceExtensionManager
+          await serviceConnection.serviceManager.serviceExtensionManager


do we want to await this or was this intentionally unawaited?

kenzieschmoll · 2025-04-07T21:04:32Z

packages/devtools_app/lib/src/screens/inspector_v2/inspector_controller.dart

+    addAutoDisposeListener(_supportsToggleSelectWidgetMode, () async {
      if (_supportsToggleSelectWidgetMode.value) {
-        serviceConnection.serviceManager.serviceExtensionManager
+        await serviceConnection.serviceManager.serviceExtensionManager


packages/devtools_app/lib/src/shared/utils/utils.dart

kenzieschmoll · 2025-04-07T21:06:04Z

...devtools_app/lib/src/screens/performance/panes/controls/enhance_tracing/enhance_tracing.dart

+                await _updateForServiceExtensionState(state.value, type);
+                addAutoDisposeListener(state, () async {
+                  await _updateForServiceExtensionState(state.value, type);


same question regarding whether these were intentionally unawaited

kenzieschmoll · 2025-04-07T22:57:21Z

Looking at this PR further, I worry we are changing the functionality of the code to enable this lint. Some of this code looks like it should be intentionally unawaited so that it does not block the UI thread.

elliette · 2025-04-07T23:09:42Z

Looking at this PR further, I worry we are changing the functionality of the code to enable this lint. Some of this code looks like it should be intentionally unawaited so that it does not block the UI thread.

Hmm I will go through these changes again and update the ones which like they should be unawaited. Will respond when this is ready for re-review!

elliette · 2025-04-08T18:23:10Z

packages/devtools_app/lib/src/screens/inspector_v2/inspector_screen_body.dart

      if (serviceConnection.serviceManager.connectedState.value.connected &&
          controller.firstInspectorTreeLoadCompleted) {
-        controller.refreshInspector();
+        await controller.refreshInspector();


I believe it is better to await the tree refresh here to avoid race conditions

Is the closure awaited when the listener is called in the auto dispose logic? If not, this await won't matter and we should add a comment or explicitly mark unawaited

Ah good point, it's not. Switched to unawaited

elliette · 2025-04-08T18:23:30Z

packages/devtools_app/lib/src/service/vm_service_wrapper.dart

-  Future<Success> streamCancel(String streamId) {
-    _activeStreams.remove(streamId);
+  Future<Success> streamCancel(String streamId) async {
+    await _activeStreams.remove(streamId);


This looks like it should have always been awaited

elliette · 2025-04-08T18:24:03Z

packages/devtools_shared/lib/src/service/service.dart

+    client.sink!.done.whenComplete(() async {
      finishedCompleter.complete();
-      service.dispose();
+      await service.dispose();


I think we should wait for dispose after the connection is closed here

elliette · 2025-04-08T18:24:15Z

packages/devtools_shared/lib/src/service/service.dart

+      (_) async {
        finishedCompleter.complete();
-        service.dispose();
+        await service.dispose();


Same comment as above here

elliette · 2025-04-08T18:25:22Z

Switched most futures from awaited to unawaited, the ones where I didn't I've left comments for why I think they should be awaited

kenzieschmoll · 2025-04-08T19:14:13Z

packages/devtools_app/lib/src/screens/inspector_v2/inspector_screen_body.dart

      if (serviceConnection.serviceManager.connectedState.value.connected &&
          controller.firstInspectorTreeLoadCompleted) {
-        controller.refreshInspector();
+        await controller.refreshInspector();


Is the closure awaited when the listener is called in the auto dispose logic? If not, this await won't matter and we should add a comment or explicitly mark unawaited

kenzieschmoll · 2025-04-08T19:15:54Z

packages/devtools_app/lib/src/shared/preferences/preferences.dart

-        _UiPreferences.vmDeveloperMode.storageKey,
-        '${vmDeveloperModeEnabled.value}',
+      safeUnawaited(
+        storage.setValue(


do we ever await storage.setValue? If not, we could change the return type of setValue to void and then add the safeUnawatied wrapper inside that method.

Good point, setValue now returns void instead of Future<void>

It looks like we were awaiting storage.setValue in some places though. My comment was only if we weren't awaiting this method call anywhere but it looks like we were. I don't know if we want to change the behavior of places where we were awaiting.

Ah got it! Reverted those changes

kenzieschmoll

lgtm with one question

kenzieschmoll · 2025-04-08T23:52:19Z

...devtools_app/lib/src/screens/performance/panes/controls/enhance_tracing/enhance_tracing.dart

        serviceConnection.serviceManager.serviceExtensionManager
            .waitForServiceExtensionAvailable(extension.extension)
-            .then((isServiceAvailable) {
+            .then((isServiceAvailable) async {


is this async still needed?

good catch, no!

elliette added 3 commits April 7, 2025 12:40

Enable lint

13ce11b

Add safeUnawaited method

0b55d24

Fix instances of discarded_futures

3d40e17

elliette requested review from a team, bkonyi and kenzieschmoll as code owners April 7, 2025 20:24

elliette changed the title ~~Re-enabled discarded_futures lint~~ Re-enable discarded_futures lint Apr 7, 2025

elliette added the release-notes-not-required label Apr 7, 2025

kenzieschmoll reviewed Apr 7, 2025

View reviewed changes

packages/devtools_app/lib/src/shared/utils/utils.dart Show resolved Hide resolved

kenzieschmoll reviewed Apr 7, 2025

View reviewed changes

Switch awaits to unawaits

6ceeee8

elliette commented Apr 8, 2025

View reviewed changes

kenzieschmoll reviewed Apr 8, 2025

View reviewed changes

elliette added 3 commits April 8, 2025 13:13

Update storage.setValue

d350224

Don't await tree refresh

30c64ad

Partial revert of "Update storage.setValue"

ea74a31

kenzieschmoll approved these changes Apr 8, 2025

View reviewed changes

Remove async

3d1abbc

elliette merged commit c2707cf into flutter:master Apr 10, 2025
41 checks passed

hrajwade96 pushed a commit to hrajwade96/devtools that referenced this pull request Sep 2, 2025

Re-enable discarded_futures lint (flutter#9117)

a0c48b1

Conversation

elliette commented Apr 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll commented Apr 7, 2025

Uh oh!

elliette commented Apr 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elliette commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants