kotlinx.coroutines icon indicating copy to clipboard operation
kotlinx.coroutines copied to clipboard

Make withTimeout throw not a CancellationException

Open dkhalanskyjb opened this issue 2 years ago • 3 comments

Fixes #1374

dkhalanskyjb avatar Oct 31 '22 15:10 dkhalanskyjb

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.

dkhalanskyjb avatar Oct 31 '22 15:10 dkhalanskyjb

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

elizarov avatar Dec 21 '22 09:12 elizarov

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

qwwdfsad avatar Dec 27 '22 16:12 qwwdfsad