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

binder: ServerAuthInterceptor might use an Executor that BinderServer has already returned to the pool #10897

Closed
mateusazis opened this issue Feb 6, 2024 · 2 comments
Milestone

Comments

@mateusazis
Copy link
Contributor

This is a follow-up to #10566.

(note: I'm describing the issue as I recollect it; Chat ate the discussion thread. @markb74 please correct me if I'm wrong)

#10633 introduced the PendingAuthListener class and the logic to handle AsyncSecurityPolicy without blocking any threads. This required an Executor to be passed to BinderTransportSecurity here. In the same PR, we added a shutdown listener to return that executor to the object pool during server shutdown (here).

@markb74 commented that this was sub-optimal, and we could instead return the executor during termination. One suggestion was to pass an Executor to BinderServer.start and reuse it throught that server's lifetime. I prototyped it in mateusazis@3f262ca, but ended up with a chicken-and-egg issue in BinderServerBuilder.build:

  • BinderTransportSecurity.installAuthInterceptor wants an Executor, but it will only be available on BinderServer after it is instantiated and started
  • BinderServer is only instantiated after the authorization interceptor gets installed

Any help here is welcome!

@ejona86 ejona86 added the binder label Feb 13, 2024
@sergiitk sergiitk added this to the Next milestone Feb 13, 2024
@jdcormie
Copy link
Member

jdcormie commented May 25, 2024

I think I finally understand the problem Mark was pointing out.

Today BinderServer returns this Executor to the pool in InternalServer#shutdown(). But shutdown() merely "Initiates an orderly shutdown of the server. Existing transports continue ....". And ServerAuthInterceptor is invoked for every call, even on established transports. So we have a bug where ServerAuthInterceptor might use this executor after it has been released. The upshot could be RejectedExecutionExceptions, since the resource pool may call shutdown on a returned Executor.

It seems to me that we can't safely release this executor until the last BinderServerTransport created by this BinderServer has called ServerTransportListener#transportTerminated()

mateusazis added a commit to mateusazis/grpc-java that referenced this issue May 26, 2024
…rning security policy executor to the object pool.

Addresses grpc#10897.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue May 27, 2024
…rning security policy executor to the object pool.

Addresses grpc#10897.
@mateusazis
Copy link
Contributor Author

mateusazis commented May 28, 2024

Ah, that does ring a bell w.r.t. to the chat I had with him. #11240 is an attempt to address it. PTAL when you have a chance.

@jdcormie jdcormie added bug and removed enhancement labels May 29, 2024
@jdcormie jdcormie changed the title Binder: recycle PendingAuthListener's executor during transport termination binder: ServerAuthInterceptor might use an Executor that BinderServer has already returned to the pool May 29, 2024
mateusazis added a commit to mateusazis/grpc-java that referenced this issue May 30, 2024
…rning security policy executor to the object pool.

Addresses grpc#10897.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue May 31, 2024
…tart despite the server having shutdown already.

Part of grpc#10897.
@ejona86 ejona86 modified the milestones: Next, 1.65 Jun 11, 2024
larry-safran pushed a commit to larry-safran/grpc-java that referenced this issue Aug 13, 2024
... returning security policy executor to the object pool.

Fixes grpc#10897.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants