Skip to content
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

If the frame handler thread is null do not schedule it on the executor #11651

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

kannanjgithub
Copy link
Contributor

Fixes NPE from "Detect transport executors with no remaining threads #11503".

@kannanjgithub kannanjgithub requested a review from ejona86 October 29, 2024 15:03
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the latch/barrier handling needs to move into the try-catch that guarantees clientFrameHandler is set. So if the second thread fails, we'll use the source with "Use closed source on failure so that the reader immediately shuts down."

@kannanjgithub
Copy link
Contributor Author

Only the source is guaranteed to be set after a certain point, so I think you mean source and not clientFrameHandler. I have pushed a commit with an attempt, although some other tests need fixing that fail with a NPE.

@ejona86
Copy link
Member

ejona86 commented Oct 29, 2024

Assigning clientFrameHandler is part of a finally block.

} finally {
clientFrameHandler = new ClientFrameHandler(variant.newReader(source, true));

@kannanjgithub
Copy link
Contributor Author

The case causing NPE for clientFrameHandler is this latch.await() timing out and sending a go away for internal error. How could this code be moved after clientFrameHandler is initialized in finally, because I thought we had to wait for connection preface settings to be sent first.

@ejona86
Copy link
Member

ejona86 commented Oct 30, 2024

Move it into the try block, so if it fails clientFrameHandler will be initialized still via the finally.

@kannanjgithub
Copy link
Contributor Author

Got it, I wrongly assumed you were implying changing the order of the statements and got confused, you were only suggesting expanding the try scope to include the latch and barrier await. Pushed the commit.

@ejona86 ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Oct 30, 2024
@kannanjgithub kannanjgithub merged commit ef1fe87 into grpc:master Oct 31, 2024
14 of 15 checks passed
@kannanjgithub kannanjgithub deleted the okhttpnpefix branch October 31, 2024 06:23
@ejona86 ejona86 removed the TODO:backport PR needs to be backported. Removed after backport complete label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants