-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(native): Fix an NPE issue in HttpNativeExecutionTaskResultFetcher #26550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideHttpNativeExecutionTaskResultFetcher’s onSuccess handler now safely cancels the scheduledFuture by guarding against null, preventing potential NPE. Class diagram for updated HttpNativeExecutionTaskResultFetcher.onSuccess() null checkclassDiagram
class HttpNativeExecutionTaskResultFetcher {
-ScheduledFuture scheduledFuture
+void onSuccess(PageBufferClient.PagesResponse pagesResponse)
}
class PageBufferClient {
+void abortResultsAsync()
}
class PagesResponse {
+boolean isClientComplete()
}
HttpNativeExecutionTaskResultFetcher --> PageBufferClient
PageBufferClient --> PagesResponse
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hantangwangd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xin-zhang2 thanks for this fix, basically looks good to me.
This seems to be a race condition: an async task is scheduled immediately to potentially cancel the result future of this scheduling operation (which may not be assigned yet). Not very sure, but do you think it's reasonable to change the initialDelay to a value bigger than 0 in the following code, since the scheduled task doGetResults may try to cancel ScheduledFuture<?> future before it's assigned:
ScheduledFuture<?> future = scheduler.scheduleAtFixedRate(this::doGetResults,
0,
(long) FETCH_INTERVAL.getValue(),
FETCH_INTERVAL.getUnit());
wdyt: @shrinidhijoshi @xin-zhang2
Description
Fixes a potential NPE issue in HttpNativeExecutionTaskResultFetcher.
See #22338
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.