fix NPE and other exceptions thrown when work fails to process before work execution #32566
Conversation
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| } | ||
|
|
||
| public Windmill.WorkItem getWorkItem() { | ||
| return work.getWorkItem(); | ||
| return checkNotNull(work).getWorkItem(); |
There was a problem hiding this comment.
I would just leave it as work.getWorkItem, NullpointerException is clearer than IllegalArgumentException
There was a problem hiding this comment.
checker will complain since we mark work field as Nullable now
this will still throw an NPE with the checkNotNull check
i added a more clear error message.
| try { | ||
| context().invalidateCache(); |
There was a problem hiding this comment.
lets keep it outside and not suppress the exception. I'm not sure about the implications of suppressing the exception here, it is better to fail than operating in an inconsistent state.
There was a problem hiding this comment.
Discussed over chat.
Safe because:
- logic on invalidate cache will not run if work is null
- Only 1 thread holds a reference to ComputationWorkExecutor and the only way it can be reused by another thread is if it is released to ComputationState. If an exception is thrown during processing, the ComputationWorkExecutor will not be reused and the reference freed.
There was a problem hiding this comment.
I think either is okay here, since even if we throw it the upper layer suppresses it. But in general it is better to not suppress unexpected exceptions and let the upper layer handle or re-throw it.
|
@scwhittle ready for merge thank you! |
|
Precommit failure is pubsub auth issue, seems unrelated. |
StreamingModeExecutionContext requires a call to
start()in order to work correctly.If an exception is thrown during work execution before a call to start(), its possible that internal StreamingModeExecutionContext members are
null.Fix NPE in calls to
invalidateCache()made w/o a call tostart().Also prevent ComputationWorkExecutor.invalidate() from crashing the worker.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.