Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Close ijson coroutines ourselves instead of letting the GC close them #13293

Closed
squahtx opened this issue Jul 15, 2022 · 4 comments · Fixed by #14065
Closed

Close ijson coroutines ourselves instead of letting the GC close them #13293

squahtx opened this issue Jul 15, 2022 · 4 comments · Fixed by #14065
Assignees
Labels
A-Federation good first issue Good for newcomers S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Sentry Issue was discovered by looking at Sentry reports on Matrix.org

Comments

@squahtx
Copy link
Contributor

squahtx commented Jul 15, 2022

def finish(self) -> SendJoinResponse:
for c in self._coros:
c.close()

def finish(self) -> StateRequestResponse:
for c in self._coros:
c.close()

These close() calls were added in #12875, but I overlooked that we'd only close the first coroutine if it raised an IncompleteJSONError.
We need to close all coroutines, even when any of them raise an error.

Until we fix this properly, we'll get unhelpful Exception ignored in: <generator object utf8_encoder at 0x7f87c9b1ccf0> errors in sentry without a stack trace or logging context.

@squahtx squahtx added A-Federation S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. good first issue Good for newcomers labels Jul 15, 2022
@DMRobertson
Copy link
Contributor

What's the fix here? Wrap each close call in a try: ... except Exception: pass?

@squahtx
Copy link
Contributor Author

squahtx commented Jul 18, 2022

What's the fix here? Wrap each close call in a try: ... except Exception: pass?

I think we'd want to let the exception bubble up still. Maybe something like

exc = None
for c in self._coros:
    try:
        c.close()
    except Exception as e:
        exc = e
if exc is not None:
    raise exc

if that doesn't mess up the exception stack traces.

@richvdh
Copy link
Member

richvdh commented Jul 18, 2022

if that doesn't mess up the exception stack traces.

I think (untested) that stack traces would be fine (if an exception already has a traceback object when you raise it, it is not replaced), but you'd need to worry about exception chaining. I note that contextlib.ExitStack has some horrid workarounds to deal with that.

@DMRobertson
Copy link
Contributor

DMRobertson commented Jul 18, 2022

(Would https://peps.python.org/pep-0654/ 's ExceptionGroups be helpful? Not that this is available to us any time soon.)

@clokep clokep added the Z-Sentry Issue was discovered by looking at Sentry reports on Matrix.org label Oct 5, 2022
@DMRobertson DMRobertson self-assigned this Oct 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Federation good first issue Good for newcomers S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Sentry Issue was discovered by looking at Sentry reports on Matrix.org
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants