DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5237307964
Thanks for the thoughtful follow-up, and glad the taskDone() point landed clearly - that was the part I most wanted to make sure didn't get lost, since it's a second, independent bug that happened to be hiding behind the same NPE as the startedLatch issue. On your question for Issue 1: I'd lean toward including the null check in recycleClassLoader() in this same PR rather than splitting it out. The reason is that this PR is precisely what makes that path more reachable - before your fix, the pre-try NPE meant taskDone() (and therefore recycleClassLoader()) was frequently never reached in the stale-context race scenario, so the exposure was accidental and inconsistent. After your fix, taskDone() is guaranteed to run from the finally block, so the same race that motivated this PR becomes more likely to reach the un-null-checked get() in recycleClassLoader(). Since the fix and the newly-increased exposure are causally linked, keeping them in one PR tells a cleaner story for anyone reading the history later, and it's a small, low-risk, purely defensive addition (null check, log a warning, return) so it shouldn't meaningfully grow the diff's risk surface. That said, this is genuinely non-blocking - if you'd rather keep this PR minimal an d ship the null check as an immediate fast-follow, that's a fine call too. I'll leave the choice to you. Sounds like a solid plan overall: test comment clarification, Javadoc update on the BlockingWorker.run() list, your call on Issue 1, and a filed follow-up for the underlying stale-taskDone()/executionContexts race linked back here and to #11679. Once those land and you take the PR out of draft, I'll do a full pass over the updated diff. Nothing here changes my earlier conclusion - this is a solid, well-verified fix for a real hang, and I'm looking forward to seeing it merged. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
