Skip to content

Avoid request threads on AppEngine Java 8#3382

Merged
ejona86 merged 1 commit intogrpc:masterfrom
ejona86:gae8-threads
Aug 24, 2017
Merged

Avoid request threads on AppEngine Java 8#3382
ejona86 merged 1 commit intogrpc:masterfrom
ejona86:gae8-threads

Conversation

@ejona86
Copy link
Member

@ejona86 ejona86 commented Aug 23, 2017

While the code had correctly determined full threads were available, the
call to MoreExecutors returned a request thread factory, which has
limitations.

Note that Async stub users may not be able to call GAE APIs in
callbacks. This is because the threads aren't request threads. They can
override the individual call's executor with
com.google.appengine.api.ThreadManager.currentRequestThreadFactory() in
an interceptor via callOptions.withExecutor().

Fixes #3296


@neozwu, can you test this?

As a user, I think I'd expect the Async API callbacks to be done or request threads. But this isn't very feasible to configure automatically because the executor would need to "save" the environment when a new call is created and re-associate callbacks with it. It'd be easy to override blindly, but then users couldn't override the executor.

While the code had correctly determined full threads were available, the
call to MoreExecutors returned a request thread factory, which has
limitations.

Note that Async stub users may not be able to call GAE APIs in
callbacks. This is because the threads aren't request threads. They can
override the individual call's executor with
com.google.appengine.api.ThreadManager.currentRequestThreadFactory() in
an interceptor via callOptions.withExecutor().

Fixes grpc#3296
@ejona86
Copy link
Member Author

ejona86 commented Aug 23, 2017

CC @garrettjonesgoogle

@neozwu
Copy link

neozwu commented Aug 24, 2017

@ejona86 with this change, the NPE is fixed. However, I got an io.grpc.StatusRuntimeException: UNAUTHENTICATED error. It seems this is due to the fact refressAccessToken is now called in a "normal" thread, and, as you mentioned, refreshAccessToken internally calls appengine API, so it will fail in "normal" thread. I will go ahead and try fix the auth lib to use metadata server instead of appengine API for authentication.

@ejona86 ejona86 requested a review from zpencer August 24, 2017 20:13
@ejona86
Copy link
Member Author

ejona86 commented Aug 24, 2017

@neozwu, thanks. We'll go ahead and merge this and backport it to the upcoming release.

@ejona86 ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Aug 24, 2017
@ejona86 ejona86 merged commit 521c55e into grpc:master Aug 24, 2017
@ejona86 ejona86 deleted the gae8-threads branch August 24, 2017 22:47
@ejona86 ejona86 removed the TODO:backport PR needs to be backported. Removed after backport complete label Aug 28, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants