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

New client PreInvocationInterceptor and PostInvocationInterceptor SPI #4301

Merged
merged 3 commits into from
Nov 21, 2019

Conversation

jansupol
Copy link
Contributor

… executed around the request

Signed-off-by: Jan Supol [email protected]

@jansupol
Copy link
Contributor Author

New SPI to provide ClientRequestContext and ClientResponseContext at the beginning and at the end of the client request invocation. The primary use case is to provide the contexts to set some properties before and after the request, i.e. before the ClientRequestFilter and ClientResponseFilter instances are processed, and to ensure that every instance of the new SPI interfaces (PreInvocationInterceptor && PostInvocationInterceptor) is processed, no matter the exception thrown in the request chain.

This can be particularly useful for logging, measuring and tracing the requests. The Filters are not useful, since they may not be processed at all, if ClientRequestContext#abortWith or Exception occur.

… executed around the request

Signed-off-by: Jan Supol <[email protected]>
Copy link
Contributor

@pdudits pdudits left a comment

Choose a reason for hiding this comment

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

This is very nice and rich API beyond what I imagined. I tried some experiments with it, and can be seen on this branch:

pdudits@7ff7e84: implementing thread-local context propagation for async() and rx() calls will be straightforward

pdudits@e9397cf: Reimplementing AsyncInterceptor without need for executorService wrapping is also simple. Only missing thing is, that the PreInvocationInterceptor doesn't know the kind of invocation that is happening, and therefore cannot be registered as top-level component, but need to wait until the client is sure, that it will invoke async request.

pdudits@ed56aca: Throwing Error in PostInvocationInterceptor hides it in suppresed exceptions. It would be better to include first throwable as a cause, as also commented on this PR.

pdudits@63c2cc3: Trying context propagation with third-party RxInvoker (RxJava) fails -- the invoker doesn't invoke the interceptor on calling thread, therefore the ultimate goal of being able to always propagate calling thread context is not met by this PR.
Do you see any way of making this possible? Or is it something that RxInvokerProviders need to support explicitly?

@jansupol
Copy link
Contributor Author

the invoker doesn't invoke the interceptor on calling thread

Maybe some other use case would make sense - to implement something similar to RestClientBuilderListener. While it could be used by external factories to set the request, it would fit your use case, too. I admit the primary use-case for PreInvocationInterceptor is a bit different.

I monitor requests for such an interface anyway, I try to come up with some PR.

Copy link
Contributor

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

LGTM

@senivam senivam merged commit cfa1734 into eclipse-ee4j:master Nov 21, 2019
@jansupol jansupol added this to the 2.30 milestone Nov 23, 2019
@jansupol jansupol self-assigned this Nov 23, 2019
@jansupol jansupol added the spi label Nov 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants