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

No API for for async context propagation in Client API #4267

Open
pdudits opened this issue Sep 20, 2019 · 4 comments
Open

No API for for async context propagation in Client API #4267

pdudits opened this issue Sep 20, 2019 · 4 comments
Labels
client_api enhancement New feature or request

Comments

@pdudits
Copy link
Contributor

pdudits commented Sep 20, 2019

Currently the client offers no API for capturing calling thread context when invocations are turned into async on RX ones. ClientRequestFilters are first executed on executing thread.

Proposal:

New component that can be registered into ClientBuilder with roughly following API:

interface AsyncContextProvider {
   void initAsyncContext(RequestContext asyncContext);
}

These providers would be looked up and invoked by AsyncInvoker or before turning an invocation into RxInvoker, to allow for capturing additional calling thread context into request context.

@pdudits pdudits changed the title API for for async context propagation in Client API No API for for async context propagation in Client API Sep 20, 2019
@jansupol jansupol added client_api enhancement New feature or request labels Oct 4, 2019
@jansupol
Copy link
Contributor

  • There is Invocation.Builder#property to set properties to be accessible on the ClientRequestFiter

  • RxInvoker has an RxProvider spi that basically makes it hard to make sure the AsyncContextProvider would run in a non-executor thread. Mostly, the RxProvider runs the SyncInvoker in a thread given by the executor service, so it is late already. An option is to run the AsyncContextProvider as soon as the async()/rx() is executed. But the request may never be actually invoked, so it does not sound right to execute that at the point.

  • For AsyncInvoker this can be done

  • For default RxInvoker this can be done, too.

@pdudits What do you think the right solution for RxInvoker would be?

@jansupol
Copy link
Contributor

I have created #4301, which supersedes this requirement. Does it look good to you?

@pdudits
Copy link
Contributor Author

pdudits commented Oct 24, 2019

Oh, forgot to react last time, the PR looks interesting, I will try it out.

@jansupol
Copy link
Contributor

@pdudits Thanks, I am open to suggestions for improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client_api enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants