Use IOApp#reportFailure instead of printStackTrace() for blocking EC
#4044
Conversation
core/jvm/src/main/scala/cats/effect/unsafe/IORuntimeCompanionPlatform.scala
Outdated
Show resolved
Hide resolved
That would be great, is there anything I could/should do about it? |
|
Yes, you can rebase your branch and retarget the PR on GitHub. |
|
I'm not sure how to handle bincompat, I've pushed a 2nd commit with an attempt to address it. |
core/jvm/src/main/scala/cats/effect/unsafe/IORuntimeCompanionPlatform.scala
Outdated
Show resolved
Hide resolved
fredfp
force-pushed
the
fix/3993
branch
2 times, most recently
from
2849699 to
99ed9df
Compare
| threads, | ||
| threadPrefix, | ||
| blockerThreadPrefix, | ||
| reportFailure = t => self.compute.reportFailure(t)) |
There was a problem hiding this comment.
Hang on, sorry. I'm just a little bit confused by this. Does this actually work?
Here's why I'm confused: this method is building the WorkStealingComputeThreadPool which will be installed as compute in self.compute. Thus we are implementing reportFailure of this ExecutionContext by delegating recursively to itself. Is that right or am I mixed up somehow?
There was a problem hiding this comment.
You are absolutely right, thanks for spotting this. I went to check pre-3.4 source code and cats.effect.IOApp#reportFailure didn't exist at the time, so I reverted to the previous behaviour which is the best we can do.
armanbilge
changed the title
IOApp#reportFailure instead of printStackTrace() where possible
| ): (ExecutionContext, () => Unit) = | ||
| createDefaultBlockingExecutionContext(threadPrefix, _.printStackTrace()) | ||
|
|
||
| private[effect] def createDefaultBlockingExecutionContext( |
There was a problem hiding this comment.
We should create a follow-up to make this public in 3.x.
| if (!NonFatal(t)) { | ||
| t.printStackTrace() | ||
| runtime.compute.reportFailure(t) |
There was a problem hiding this comment.
Hmm, I'm look at this and suddenly I'm not so sure about this change. Here's the problem.
!NonFatal(t) "not non-fatal" i.e. it is a fatal error. When there is a fatal error (e.g. OutOfMemoryError) the JVM is in an inconsistent state and we cannot assume that it is safe to allocate.
The runtime.compute.reportFailure(t) may involve allocating an IO/Fiber and therefore is not safe to do in general.
Meanwhile t.printStackTrace() is know to be always safe.
armanbilge
changed the title
IOApp#reportFailure instead of printStackTrace() where possibleIOApp#reportFailure instead of printStackTrace() for blocking EC
fixes #3993