-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Do not cache failed futures for async security policies indefinitely. #10743
Conversation
Currently, if caching is enabled (as is often the case) and AsyncSecurityPolicy returns a failed future, then this future is cached forever, without giving the SecurityPolicy implementation a chance to be retried. Going forward, new invocations will trigger new security checks if the last one could not be completed successfuly. Part of grpc#10566.
Can you share more context on this PR? I remember we discussed not having more than one async security policy check against the same uid in-flight at a time. But is there a reason to not cache negative outcomes too? How should we think about this change in the context of the SecurityPolicy javadoc that says "As long as any given UID has active processes, this method should return the same value for that UID. In order words, policy changes which occur while a transport instance is active, will have no effect on that transport instance." Thanks! |
Sure. Before this PR, any This PR addresses the case when the Please see the new unit tests in this CL; they are good examples of the behavior I'm trying to enable. Without the caching changes from this PR, these tests would fail because all requests would result in status In summary, as of this PR, we should have:
*: it's actually cached up until the next request, since I couldn't find a good way to remove the listenable future from the cache upon failure. Up to suggestions though. |
binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java
Outdated
Show resolved
Hide resolved
binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java
Outdated
Show resolved
Hide resolved
binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java
Outdated
Show resolved
Hide resolved
binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java
Show resolved
Hide resolved
binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java
Outdated
Show resolved
Hide resolved
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.
Thanks, looks good.
Currently, if caching is enabled (as is often the case) and AsyncSecurityPolicy returns a failed future, then this future is cached forever, without giving the SecurityPolicy implementation a chance to be retried. Going forward, new invocations will trigger new security checks if the last one could not be completed successfuly.
Part of #10566.