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

Add context propagation support for direct Thread creation #2527

Open
iNikem opened this issue Mar 8, 2021 · 6 comments
Open

Add context propagation support for direct Thread creation #2527

iNikem opened this issue Mar 8, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@iNikem
Copy link
Contributor

iNikem commented Mar 8, 2021

We support context propagation when Runnable or Callable is submitted for execution to some executor service. But this does not work with the most primitive way to dispatch a unit of work into a separate thread: new Thread(Runnable).start(). Should add this instrumentation as well.

@iNikem iNikem added the enhancement New feature or request label Mar 8, 2021
@trask
Copy link
Member

trask commented Apr 5, 2021

this will make our context leaking problems worse...

@JonasKunz
Copy link
Contributor

I'm wondering whether this issue will gain importance with the arrival of virtual threads?

Prior to virtual threads, it was more of an antipattern to use new Thread(...).start() instead of ExecutorServices: threads are expensive and slow to start.

This changes now with virtual threads. It might become more common do the following:

Thread task = Thread.startVirtualThread(() -> ...)
...
task.join();

At the same time, the ExecutorService approach remains relevant if you are interested in querying the results of the asynchronous task, which already is covered by existing instrumentations:

try(var executor = Executors.newVirtualThreadPerTaskExecutor()) {
   Future<?> result1 = executor.submit(...);
   Future<?> result2 = executor.submit(...);
} //waits for all submitted tasks to terminate

Do you have any opinions here?

@mateuszrzeszutek
Copy link
Member

We might actually need to modify the executors instrumentation a bit, since Executors.newVirtualThreadPerTaskExecutor() uses a new ThreadPerTaskExecutor executor service implementation.

For the direct Thread usage: I think we could instrument just the new virtual thread methods, without touching any of the platform thread stuff.

@JonasKunz
Copy link
Contributor

For the direct Thread usage: I think we could instrument just the new virtual thread methods, without touching any of the platform thread stuff.

We've been thinking about the same thing with the elastic APM agent. However, even with virtual threads the point against it about context leakage remains:
E.g. imagine a service where a span is created to capture the startup procedure of this service.
The startup procedure includes spinning up a Thread (virtual or non-virtual) for accepting HTTP-connections. You would definitely not want to have the context from the initial startup propagate to the HTTP request handling I assume.

An option we considered is to provide an instrumentation for Thread.start() based instrumentation, but to make it configurable:

  • Perform context propagation for all thread
  • Perform context propagation only for virtual Threads
  • Do not propagate context

This gives users the choice, but it still is difficult to come up with a reasonable default.

@mateuszrzeszutek
Copy link
Member

The startup procedure includes spinning up a Thread (virtual or non-virtual) for accepting HTTP-connections. You would definitely not want to have the context from the initial startup propagate to the HTTP request handling I assume.

Aren't virtual threads supposed to be used for short-lived tasks though? If you start a new virtual thread for the purpose of handling HTTP connections that seems to kinda defeat the purpose (you could use a standard thread pool instead after all).

Anyway, you've a good point though -- perhaps we should just wait a bit and see what usage patterns emerge after a while?

@JonasKunz
Copy link
Contributor

Aren't virtual threads supposed to be used for short-lived tasks though? If you start a new virtual thread for the purpose of handling HTTP connections that seems to kinda defeat the purpose (you could use a standard thread pool instead after all).

To my understanding virtual threads are not limited to short-lived tasks. Short-lived tasks are just the key selling point for virtual threads where platform threads previously couldn't be used.

From a Java user perspective, the only remaining benefit of platform threads is fairness in case of CPU-contention: Your platform thread is guaranteed to run at some point in time due to preemption of platform threads. Virtual threads in contrast can hog the CPU if they never finish and never block, causing other virtual threads to never run. I would however say that this is not your typical usage scenario in an HTTP server.

Maybe a service receiving message from multiple message sources / queues is a better example:
Imagine a service who consumes messages from thousands of sources: On startup, you create a virtual thread for each source. Each thread simply does a blocking read on its message source in a while(true) loop. This would not be possible efficiently with platform threads.

perhaps we should just wait a bit and see what usage patterns emerge after a while

Yeah I'm feeling the same way.

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

No branches or pull requests

4 participants