Repository navigation
Add gRPC request scope support (#392) - #436
Conversation
5ab5166 to
df19612
Compare
|
We don't have auto-configuration in this project so the naming is wrong. I also think it would be better to stick those bean definitions in an |
|
Hi @dsyer! Thanks for the valuable feedback. I've updated the PR to remove the old auto-configuration and implemented your suggestion using the @EnableGrpcScope annotation along with the ImportBeanDefinitionRegistrar. Ready for another review! |
5fed8f4 to
bc4ad85
Compare
|
That's good, thanks. We'd still like an annotation for bean definitions (it would have to go in a different package since the name will clash). Have a look at |
|
Hi @dsyer, I've updated the implementation to isolate the bean definition logic into the EnableGrpcRequestScope annotation (along with the registrar) and added the requested integration tests modeled after Spring Web's request scope tests. Ready for another review when you have a moment. |
|
I'm sorry I don't understand. Why do we have 2 |
Signed-off-by: Your Name <you@example.com> Signed-off-by: Fernando Rodrigues Ripol <fernandorodriguesripol@gmail.com>
…onRegistrar Signed-off-by: Fernando Ripol <fernandorodriguesripol@gmail.com> Signed-off-by: Fernando Rodrigues Ripol <fernandorodriguesripol@gmail.com>
ee012e8 to
2c2a10c
Compare
Signed-off-by: Fernando Rodrigues Ripol <fernandorodriguesripol@gmail.com>
|
You deleted the test? |
…rget and style Signed-off-by: Fernando Rodrigues Ripol <fernandorodriguesripol@gmail.com>
|
Hi @dsyer, Apologies for the previous mix-up! The test file was accidentally dropped during a commit cleanup. I have restored and updated GrpcRequestScopeTests under org.springframework.grpc.server.annotation using a non-final target bean for CGLIB proxying. The build, checkstyle, and integration tests have all passed locally, and all commits are properly signed off. Ready for your review! |
Hold the beans of a call in its io.grpc.Context rather than in a ThreadLocal, so that every thread the context is propagated to sees the same instances and nothing is created outside of a call. Run the destruction callbacks when the context is cancelled, which covers completion and cancellation alike. Register the scope with the bean factory through a CustomScopeConfigurer and make the interceptor a global one that runs first, so that the scope is open for the other interceptors in the chain. Cover the scope with in-process call tests and document it. See spring-projectsgh-392 Signed-off-by: Sharang Gupta <sharang@sharanggupta.dev>
sharanggupta
left a comment
sharanggupta
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @fernandoripol. I had a parallel branch for #392 and compared the two by running a few request-scope tests against this branch (an in-process call with a DisposableBean scoped bean). Three things stand out, noted on the lines concerned with what the tests showed. They are all small to fix.
If it helps, I have opened fernandoripol#1 against your branch with a Context-keyed scope, the destruction callbacks, the scope registration and the tests, so it can land here rather than as a follow-up; take it or leave it.
| */ | ||
| public class GrpcRequestScope implements Scope { | ||
|
|
||
| private static final ThreadLocal<Map<String, Object>> threadLocalScope = ThreadLocal.withInitial(HashMap::new); |
There was a problem hiding this comment.
A ThreadLocal makes the scope per thread rather than per call. gRPC runs a call's listener callbacks on its executor, so successive callbacks of one call can land on different threads, and a service that hands work to another thread gets a different instance there: in my test, a lookup from a worker thread via Context.current().wrap(...) returned a second instance, and that instance was still returned on the worker thread after the call had completed, since clear() only runs on the completing thread. Keying the beans on the call's io.grpc.Context instead (a Context.Key, attached with Contexts.interceptCall(...) in the interceptor) gives one instance per call on every thread the context is propagated to; that is what the net.devh implementation does. Also, withInitial(HashMap::new) means a lookup outside a call quietly creates a bean in a thread-wide map instead of failing with ScopeNotActiveException like @RequestScope does; the test for that passed without throwing.
| } | ||
|
|
||
| @Override | ||
| public void registerDestructionCallback(String name, Runnable callback) { |
There was a problem hiding this comment.
Scoped beans never get their @PreDestroy/DisposableBean callbacks (destroy() was not called after the call completed in my test): the callbacks registered here have to be run when the call ends. A Context listener fires on completion and cancellation alike, which also saves the two overrides in the interceptor.
| @Override | ||
| public void registerBeanDefinitions(AnnotationMetadata importingClassMetadata, BeanDefinitionRegistry registry) { | ||
| if (!registry.containsBeanDefinition("grpcRequestScope")) { | ||
| registry.registerBeanDefinition("grpcRequestScope", new RootBeanDefinition(GrpcRequestScope.class)); |
There was a problem hiding this comment.
Defining a GrpcRequestScope bean does not register it as a scope: with @EnableGrpcScope alone, the first use of a @GrpcRequestScope bean fails with IllegalStateException: No Scope registered for scope name 'grpcRequest'. The registrar needs a CustomScopeConfigurer, or the interceptor can implement BeanFactoryPostProcessor and call registerScope.
| @Test | ||
| void testGrpcRequestScopeRegistration() { | ||
| try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(TestConfig.class)) { | ||
| assertThat(context.containsBean("testBean")).isTrue(); |
There was a problem hiding this comment.
containsBean never resolves the proxy, so this passes without the scope being registered. A test that makes an in-process call (InProcessServerBuilder is already a test dependency), reads the bean twice within it and once from a worker thread, and checks the instance is destroyed afterwards would cover the behaviour; the PR linked above has one you are welcome to take.
Key the gRPC request scope on the call's Context
|
Hi @sharanggupta and @dsyer! Thank you so much for the thorough review and for opening the PR against my branch, @sharanggupta. Keying the request scope on gRPC's I have merged the PR into Ready for review! |
This PR adds support for a custom gRPC request scope (
@Scope("grpcRequest")) to the Spring gRPC framework, resolving issue #392.Changes:
GrpcRequestScope: Implements the SpringScopeinterface usingThreadLocalto manage request-scoped beans lifecycle.GrpcRequestScopeServerInterceptor: A gRPCServerInterceptorthat cleans up the thread-local context upon call completion or cancellation.GrpcRequestScopeAutoConfiguration: Auto-configuration class to register both the scope and its server interceptor automatically.