Enable unused-awaitable error for mypy by MatMaul · Pull Request #14519 · matrix-org/synapse

MatMaul · 2022-11-22T16:35:27Z

Since I got bitten already twice by this, let's see if this check makes sense.

This is to be reviewed on a per commit basis.
Each commit is handling a different type of ignorable reported error.

There are also a couple of commits that look more like small actual bugs than false positives.

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file.
Pull request includes a sign off
Code style is correct
(run the linters)

MatMaul · 2022-11-22T16:39:01Z

It seems like usually # type: ignore comments have an explanation comment with it.

Do we want to do the same here or relying on the git history to find this info looks good enough ?

EDIT: I am planning to not squash this PR to keep the rationales around in the commit messages.

DMRobertson · 2022-11-22T16:45:50Z

I think a few of these are functions which are decorated by wrap_as_background_process; I don't think it's a problem that their invocations are missing an await?

In general it seems like this has a fairly high false positive rate, which is sad to see.

clokep · 2022-11-22T16:46:32Z

Do we want to do the same here or relying on the git history to find this info looks good enough ?

EDIT: I am planning to not squash this PR to keep the rationales around in the commit messages.

It looks like the git history just says "ignore these" -- I think squashing those all together would be fine.

I suspect it would be better to include the reasoning in comments in the code or add comments to run_background_process and friends to explain this.

MatMaul · 2022-11-22T17:02:09Z

I think a few of these are functions which are decorated by wrap_as_background_process; I don't think it's a problem that their invocations are missing an await?

Indeed not sure why I didn't noticed. So no bugs, I'll include them in the relevant commit.

It looks like the git history just says "ignore these" -- I think squashing those all together would be fine.

I should add more details in the commit messages to explain the rationale of each type of changes.

I suspect it would be better to include the reasoning in comments in the code or add comments to run_background_process and friends to explain this.

That makes complete sense for run_background type of ignores where I can just comment those methods.

For e39d757 it's however a bit more annoying since it is the same rationale but in quite a number of different places.
I could keep this commit separated, put a detailed explanation in the commit message and add a one liner please see detailed explanation in the commit message for each instance ?

…ered

clokep · 2023-01-25T19:21:23Z

@MatMaul Is this worth following-up with or should we close it?

DMRobertson · 2023-01-31T11:32:45Z

@MatMaul Is this worth following-up with or should we close it?

Per #14947 (comment), my vote is for landing this.

…itable

DMRobertson · 2023-02-06T13:23:35Z

I've merged in develop and will fixup what I can.

DMRobertson

I think this looks good, but I did do some of this myself---so would like another set of eyes.

AFAICS there are three categories of warnings we're suppressing here:

run_in_background and run_as_background_process return deferreds so you can block until---or do something after---the background task completes.
we call a method on a Deferred which returns self, and mypy complains that the return value is unused
synchronous ops that return a Deferred (e.g. popping from a Dict[str, Deferred])

The first class causes a lot of noise here. I wonder if we should make run_in_background and run_as_background_process return None---I don't think we ever use their return values. (And if we did we could have two variants, e.g. run_in_background_and_discard versus run_in_background_and_ which does return a deferred, say run_in_background_returning_handler` or something.)

changelog.d/14519.misc

DMRobertson · 2023-02-06T19:34:27Z

synapse/_scripts/update_synapse_database.py

+        defer.ensureDeferred(  # type: ignore[unused-awaitable]
            run_as_background_process("background_updates", run_background_updates)
        )


Is the call to ensureDeferred actually doing anything here? run_as_background_process returns a Deferred and ensureDeferred is a no-op when its argument is a Deferred.

(I also find this script really confusing---the way it starts the reactor and gets the task being run to stop the reactor!)

I suspect that we can remove the ensureDeferred call.

DMRobertson · 2023-02-06T19:46:14Z

synapse/metrics/_gc.py


    gc_task = task.LoopingCall(_maybe_gc)
-    gc_task.start(0.1)
+    gc_task.start(0.1)  # type: ignore[unused-awaitable]


This returns a deferred which fires when the gc_task.stop() is called---which it never is---or if the task fails. Seems safe to ignore.

DMRobertson · 2023-02-06T19:51:08Z

synapse/util/async_helpers.py

                # `new_defer.callback()` call above.
                if self.key_to_current_writer.get(key) == new_defer:
-                    self.key_to_current_writer.pop(key)
+                    self.key_to_current_writer.pop(key)  # type: ignore[unused-awaitable]


Not sure what is going on here. I think this is safe?

Thoughts:

I wonder if del self.key_to_current_writer.pop[key] would work here?

Is it possible for self.key_to_current_writer.get(key) to change between line 637 and 638? (I don't think so---there shouldn't be another thread accessing this?)

I wonder if del self.key_to_current_writer.pop[key] would work here?

Looks like it should?

Is it possible for self.key_to_current_writer.get(key) to change between line 637 and 638? (I don't think so---there shouldn't be another thread accessing this?)

I don't think so either. The ReadWriteLock is designed to be async-safe, but not thread safe.

DMRobertson · 2023-02-06T20:03:41Z

tests/handlers/test_oidc.py

        h = self.provider

-        def force_load_metadata() -> Awaitable[None]:
+        def force_load_metadata() -> "OpenIDProviderMetadata":


This one is mine. I think this fix is correct(?)

DMRobertson · 2023-02-06T20:04:58Z

tests/http/test_matrixfederationclient.py


    def test_client_sends_body(self):
-        defer.ensureDeferred(
+        defer.ensureDeferred(  # type: ignore[unused-awaitable]


Should this have a self.get_success around it? Or is the self.pump below sufficient to ensure this gets called?

ensureDeferred will run the async function up until the first blocking await, ie. it should be fine.

DMRobertson · 2023-02-06T20:06:10Z

tests/storage/test_appservice.py

        events = [Mock(event_id="e1"), Mock(event_id="e2")]
        txn_id = 5
-        self._set_state(self.as_list[0]["id"], ApplicationServiceState.UP)
+        self.get_success(


Also not sure if this needs to be here? 🤷

Look reasonable!

DMRobertson · 2023-02-06T20:11:20Z

tests/util/caches/test_deferred_cache.py

        cache: DeferredCache[str, int] = DeferredCache("test")
        with self.assertRaises(KeyError):
-            cache.get("foo")
+            cache.get("foo")  # type: ignore[unused-awaitable]


AFAICS cache is roughly a clever Mapping[str, Deferred] throughout this file, and we're checking that we can get and set its contents. It's okay that we're not awaiting on them. But please double-check this.

squahtx · 2023-02-06T21:17:13Z

I think this looks good, but I did do some of this myself---so would like another set of eyes.

AFAICS there are three categories of warnings we're suppressing here:

run_in_background and run_as_background_process return deferreds so you can block until---or do something after---the background task completes.

The first class causes a lot of noise here. I wonder if we should make run_in_background and run_as_background_process return None---I don't think we ever use their return values. (And if we did we could have two variants, e.g. run_in_background_and_discard versus run_in_background_and_ which does return a deferred, say run_in_background_returning_handler` or something.)

nb: we're supposed to await run_in_background, otherwise CPU usage accounting gets lost when the background task outlives the request that started it. We also get wonderful "Re-starting finished log context" log spam when that happens.

DMRobertson · 2023-02-06T21:35:46Z

b: we're supposed to await run_in_background, otherwise CPU usage accounting gets lost when the background task outlives the request that started it. We also get wonderful "Re-starting finished log context" log spam when that happens.

Huh, thanks. That's news to me---the docstring at

synapse/synapse/logging/context.py

Lines 797 to 800 in d8cc86e

    
               Note that if you completely discard the result, you should make sure that 
        
               `f` doesn't raise any deferred exceptions, otherwise a scary-looking 
        
               CRITICAL error about an unhandled error will be logged without much 
        
               indication about where it came from.

made it sound like it was kind-of-okay to not await run_in_background.

DMRobertson · 2023-02-06T22:35:15Z

we're supposed to await run_in_background

Do you mean specifically using the await keyword here, or more generally awaiting by feeding it into Twisted's event loop?

Also, are the examples of using run_in_background in https://matrix-org.github.io/synapse/latest/development/synapse_architecture/cancellation.html#uncancelled-processing correct?

squahtx · 2023-02-07T00:52:26Z

we're supposed to await run_in_background

Do you mean specifically using the await keyword here, or more generally awaiting by feeding it into Twisted's event loop?

the await keyword

Also, are the examples of using run_in_background in https://matrix-org.github.io/synapse/latest/development/synapse_architecture/cancellation.html#uncancelled-processing correct?

Good question. I think those are fine because there's an implicit wait for the task to finish (or at least hit the to_resolve.callback(None) line, after which there's no more async code).

... actually maybe those examples aren't okay. They'll certainly not re-start any finished logging contexts, but the active logging context will get marked as finished inside the to_resolve.callback(None) line, before it has been deactivated. Logging contexts are way too confusing.

squahtx

In principle I like the idea of turning this check on. But there are an awful large number of false positives, which are easy to get fatigued by. I'm also fairly sure we shouldn't be discarding the result of run_in_background.

squahtx · 2023-02-07T20:28:18Z

synapse/_scripts/update_synapse_database.py

+        defer.ensureDeferred(  # type: ignore[unused-awaitable]
            run_as_background_process("background_updates", run_background_updates)
        )


I suspect that we can remove the ensureDeferred call.

squahtx · 2023-02-07T20:36:05Z

synapse/util/async_helpers.py

                # `new_defer.callback()` call above.
                if self.key_to_current_writer.get(key) == new_defer:
-                    self.key_to_current_writer.pop(key)
+                    self.key_to_current_writer.pop(key)  # type: ignore[unused-awaitable]


I wonder if del self.key_to_current_writer.pop[key] would work here?

Looks like it should?

Is it possible for self.key_to_current_writer.get(key) to change between line 637 and 638? (I don't think so---there shouldn't be another thread accessing this?)

I don't think so either. The ReadWriteLock is designed to be async-safe, but not thread safe.

squahtx · 2023-02-07T20:38:03Z

tests/http/test_matrixfederationclient.py


    def test_client_sends_body(self):
-        defer.ensureDeferred(
+        defer.ensureDeferred(  # type: ignore[unused-awaitable]


ensureDeferred will run the async function up until the first blocking await, ie. it should be fine.

squahtx · 2023-02-07T20:39:34Z

tests/storage/test_appservice.py

        events = [Mock(event_id="e1"), Mock(event_id="e2")]
        txn_id = 5
-        self._set_state(self.as_list[0]["id"], ApplicationServiceState.UP)
+        self.get_success(


Look reasonable!

Co-authored-by: David Robertson <davidr@element.io>

…itable

MatMaul · 2023-03-13T14:32:48Z

I'm also fairly sure we shouldn't be discarding the result of run_in_background.

I am unsure what to do about that.
I think in most case where we don't currently use the result we want to switch to run_as_background_process ?
I am inferring that after reading the docstrings, since run_as_background_process explicitly mentions the case (or for firing-and-forgetting in the middle of a normal synapse async function).

squahtx · 2023-03-13T17:38:14Z

I'm also fairly sure we shouldn't be discarding the result of run_in_background.

I am unsure what to do about that. I think in most case where we don't currently use the result we want to switch to run_as_background_process ?

That's probably the best way forward right now. Though I'd note that this has some implications regarding where resource usage is accounted under in metrics, and also makes it harder to determine which request kicked off which bit of processing.

DMRobertson · 2023-03-30T16:13:17Z

We've had this on our board for a while and I don't think we have the bandwidth to drive it forward. I'd love to see this fixed and I think we've learned useful information from this, but I think we should park this for now.

If anyone gets time to look at this in the future, I'd suggest tackling one of the classes identified in #14519 (review) to hopefully make a smaller PR.

github-actions bot deployed to PR Documentation Preview November 22, 2022 16:36 View deployment

MatMaul changed the title ~~Enable unused-awaitable for mypy~~ Enable unused-awaitable error for mypy Nov 22, 2022

MatMaul force-pushed the mv/mypy-unused-awaitable branch from 3f982e3 to 04c726d Compare November 22, 2022 16:48

github-actions bot deployed to PR Documentation Preview November 22, 2022 16:49 View deployment

Enable unused awaitable error in mypy

6e62451

Mathieu Velten added 2 commits November 22, 2022 18:09

Add type ignore unused-awaitable to run_as_background_process calls

a0dfa69

Add type ignore unused-awaitable to run_in_background calls

b0603da

MatMaul force-pushed the mv/mypy-unused-awaitable branch from 04c726d to 53ca28a Compare November 22, 2022 17:09

Mathieu Velten added 5 commits November 22, 2022 18:10

Add type ignore to calls of fcts using wrap_as_background_process

dc201b1

Add type ignore unused-awaitable to ensureDeffered calls

adc7903

Add type ignore unused-awaitable to places adding callbacks to a deff…

b1be9a7

…ered

Add type ignore unused-awaitable to looping_call immediate call

6ac2f57

Add type ignore unused-awaitable to LoopingCall start call

312a754

github-actions bot deployed to PR Documentation Preview November 22, 2022 17:10 View deployment

Reformat

53ca28a

github-actions bot deployed to PR Documentation Preview November 22, 2022 17:12 View deployment

Manual format of md example

4b9c67b

DMRobertson mentioned this pull request Jan 31, 2023

Fix "Re-starting finished log context" spam when creating events #14947

Merged

David Robertson added 3 commits February 6, 2023 12:55

Merge remote-tracking branch 'origin/develop' into mv/mypy-unused-awa…

be45edb

…itable

Merge remote-tracking branch 'origin/develop' into mv/mypy-unused-awa…

6f8080e

…itable

Fix force_load_metadata return type

d68b11c

github-actions bot deployed to PR Documentation Preview February 6, 2023 13:24 View deployment

Missing get_success-es

3ad65ac

David Robertson added 3 commits February 6, 2023 13:55

Ignore false positives from run_as_background_process

665156f

DeferredCache.get & .set are fine to not await(?)

ac55932

Ignore other false-positives

2dfccdc

github-actions bot deployed to PR Documentation Preview February 6, 2023 14:36 View deployment

DMRobertson marked this pull request as ready for review February 6, 2023 14:54

DMRobertson requested a review from a team as a code owner February 6, 2023 14:54

DMRobertson reviewed Feb 6, 2023

View reviewed changes

squahtx requested a review from a team February 6, 2023 21:18

squahtx reviewed Feb 7, 2023

View reviewed changes

DMRobertson mentioned this pull request Mar 13, 2023

Ensure fed-sender catchup does not block for full state #15248

Merged

Mathieu Velten and others added 2 commits March 13, 2023 12:54

Update changelog.d/14519.misc

4f135c3

Co-authored-by: David Robertson <davidr@element.io>

Merge remote-tracking branch 'origin/develop' into mv/mypy-unused-awa…

5980756

…itable

github-actions bot deployed to PR Documentation Preview March 13, 2023 13:11 View deployment

DMRobertson closed this Mar 30, 2023

DMRobertson mentioned this pull request Sep 4, 2023

Don't wake up destination transaction queue if they're not due for retry. #16223

Merged

Uh oh!

Conversation

MatMaul commented Nov 22, 2022

Pull Request Checklist

Uh oh!

MatMaul commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DMRobertson commented Nov 22, 2022

Uh oh!

clokep commented Nov 22, 2022

Uh oh!

MatMaul commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clokep commented Jan 25, 2023

Uh oh!

DMRobertson commented Jan 31, 2023

Uh oh!

DMRobertson commented Feb 6, 2023

Uh oh!

DMRobertson left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

squahtx commented Feb 6, 2023

Uh oh!

DMRobertson commented Feb 6, 2023

Uh oh!

DMRobertson commented Feb 6, 2023

Uh oh!

squahtx commented Feb 7, 2023

Uh oh!

squahtx 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatMaul commented Mar 13, 2023

Uh oh!

squahtx commented Mar 13, 2023

Uh oh!

DMRobertson commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MatMaul commented Nov 22, 2022 •

edited

Loading

MatMaul commented Nov 22, 2022 •

edited

Loading