fix: Bound repeated thread-pool loop failures - #17192
Conversation
|
This is not a trivial change. Could you explain the reasoning for this patch? What exactly happens when reaching the failure threshold? Why did you choose this fix and would there be alternatives? While putting a limit doesn't seem like a bad idea, it only treats a symptom and doesn't fix the underlying issue of #17165 |
|
Thanks for the attempt, but:
Still, the rescue/print error then keep looping was a bad choice. The loop must never fail. If it does fail, we must just panic, not even retry, unless maybe in the sure case that we know there's nothing (no attached scheduler or scheduler being attached in parallel, no lost fiber, no pending async op, not the main thread, ...). |
Per review: the main thread must not return, since that invalidates the main fiber's stack, and on Windows no thread may return while async operations are pending. Retrying a crashed loop was the wrong recovery, so drop the failure counter and the conditional return and exit after printing the error, matching Crystal::Scheduler#fatal_resume_error. The parked_for_checkout guard existed only to avoid re-pushing the parked entry before a retry, so it goes with the retries. Drops the spec added earlier in this PR, which covered the removed retry behaviour. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
514af71 to
e2d7990
Compare
|
Dropped the retry entirely: the loop now exits after printing the error, matching Crystal::Scheduler#fatal_resume_error. Nothing returns from enter_thread_loop any more, so both the main-fiber stack and the pending Windows I/O concerns go away. That collapses this to a one line change, and I removed the spec I had added since it only covered the retry path that is gone. |
Fixes #17165
Add a 👍 reaction to pull requests you find important.