Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make withTimeout throw not a CancellationException #3515

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

dkhalanskyjb
Copy link
Collaborator

Fixes #1374

@dkhalanskyjb
Copy link
Collaborator Author

dkhalanskyjb commented Oct 31, 2022

Report of how this went:

  • The IDE managed to correctly replace the deprecated withTimeout with the new overload. But! It replaces withTimeout with the fully-qualified path.
  • When replacing TimeoutCancellationException with TimeoutException, the IDE did not suggest replacing all the instances. Tought to say whether this is good. In our case, it wouldn't be. Also, there's the possible pattern catch (e: CancellationException) { if (e is TimeoutCancellationException) X() else Y() }, which would break if TimeoutCancellationException is blindly replaced everywhere.

The manual intervention by me was minimal (see the individual commits), but it seems that only a couple of tests have failed, and those are the stacktrace recovery tests. Not immediately obvious to me what's going on there.

@elizarov
Copy link
Contributor

How about trying to find a different name for a new withTimeout implementation?

@qwwdfsad
Copy link
Member

If we decide to follow a road with a new name, I suggest making it consistent with a potential replacement of withContext*.

  • -- it allows to break of structured concurrency in a bit unexpected ways (e.g. we probably should've prohibited withContext(Job()) and its behaviour with CopyableThreadContextElement may (not should) be reconsidered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants