zeroc-ice / ice

All-in-one solution for creating networked applications with RPC, pub/sub, server deployment, and more.
https://zeroc.com
GNU General Public License v2.0
2.03k stars 592 forks source link

Remove Ice thread pool synchronization context in C#? #2668

Closed bernardnormier closed 4 days ago

bernardnormier commented 3 weeks ago

The Ice thread pool sets a synchronization context for the "worker threads" it creates:

https://github.com/zeroc-ice/ice/blob/1ae9e22a64a2f8c80bdf5ad280f71064a4000c64/csharp/src/Ice/Internal/ThreadPool.cs#L796

See also: https://doc.zeroc.com/ice/3.7/language-mappings/c-sharp-mapping/client-side-slice-to-c-sharp-mapping/asynchronous-method-invocation-ami-in-c-sharp/ami-in-c-sharp-with-tasks#id-.AMIinCSharpwithTasksv3.7-ConcurrencySemanticsforAMIinC#

As a result, when you make an async invocation from an Ice thread pool thread, the async "continuation" executes in a thread pool thread. For example:

public async Task SomeDispatchMethodAsync(....)
{
    // runs in server thread pool thread
    await proxy.opAsync(); // no ConfigureAwait(false), i.e. capture synchronization context
    // back in a server thread pool thread
}

I don't see the advantage of going back into a server thread pool thread here. If it's about starving the thread pool for flow control, that's not something we want to do.

Should we remove this thread pool synchronization context?

bernardnormier commented 3 weeks ago

Note that this behavior:

When using async and await, the concurrency semantics are determined by the synchronization context in which you're making the proxy invocation. For example, awaiting an asynchronous proxy invocation from the main thread will invoke the continuation from a .NET thread pool thread.

currently depends on setting a synchronization context on the threads of the Ice client thread pool. Without this synchronization context, the continuation would execute in the thread completing the TaskCompletionSource, namely a client thread pool thread. We don't want client thread pool threads running await continuations since there is typically only one such thread.

If we remove the synchronization context from the Ice thread pools, we will also need to fix the TaskCompletionSource to run continuations asynchronously.

bentoi commented 2 weeks ago

Note that this behavior currently depends on setting a synchronization context on the threads of the Ice client thread pool. Without this synchronization context, the continuation would execute in the thread completing the TaskCompletionSource, namely a client thread pool thread.

I don't really understand what you mean here and the relationship with the client thread pool thread synchronization context.

To me, for an await invocation task configured with ConfigureAwait(true) (the default):

However, if the application does something like: proxy.sayHelloAsync().ContinueWith(() => { ... }) the continuation will run on the client thread pool thread (and eventually synchronously if I recall correctly).

If an application does something like:

proxy.sayHelloAsync().ContinueWith(async () => { await Task.Delay(10); });

The continuation of the Task.Delay call will run on an Ice client thread pool thread.

I think the goal of the synchronization context setting on the Ice thread pool threads was to be ensure awaited task from thread pool threads always used Ice threads, just like await task from a GUI thread will run on the GUI thread.

Keeping this behavior is debatable. We could instead rely on the .NET thread pool for awaited tasks.

bernardnormier commented 2 weeks ago

I don't really understand what you mean here and the relationship with the client thread pool thread synchronization context.

The current implementation of async invocations uses a TaskCompletionSource constructed with its parameterless constructor, which means that continuations typically run in the thread that performs the completion.

So if you call:

// from a thread without any synchronization context

// make remove invocation using Ice
await proxy.opAsync();

// in which thread am I?

Given our current TCS implementation, I was concerned the "continuation thread" was an Ice client thread pool thread. This would be bad. However, it's not the case because all the thread pool threads have a synchronization context.

I think the goal of the synchronization context setting on the Ice thread pool threads was to be ensure awaited task from thread pool threads always used Ice threads, just like await task from a GUI thread will run on the GUI thread.

Keeping this behavior is debatable. We could instead rely on the .NET thread pool for awaited tasks.

I don't find this behavior meaningful or helpful. Say you call:

// from an async dispatch
await proxy.opAsync();

We then jump though various threads:

Why does it matter that async dispatch executes a server/OA thread pool thread?

The continuation of the Task.Delay call will run on an Ice client thread pool thread.

I am not following. In your code sample, there is no code after the Task.Delay, so its continuation doesn't matter. Also, nobody should use "ContinueWith" in modern C#.