ConnectAsync and ShutdownAsync refactoring #1434

bernardnormier · 2022-06-22T19:32:28Z

This PR refactors ConnectAsync, ShutdownAsync and DisposeAsync (when applicable) in ClientConnection, ServerConnection, ResumableClientConnection, ConnectionPool and Server.

It's only a partial fix - the full fix requires protocol connection refactoring, in one or more follow-up PRs.

ClientConnection/ServerConnection DisposeAsync is no longer an alias for ShutdownAsync, in particular ShutdownAsync can now throw exceptions and its cancellation token behaves like a normal cancellation token.

See #1413 and #1417.

…t-async

src/IceRpc/ClientConnection.cs

src/IceRpc/ConnectionExceptions.cs

src/IceRpc/ConnectionPool.cs

src/IceRpc/ClientConnection.cs

src/IceRpc/ConnectionPool.cs

externl · 2022-06-22T20:53:40Z

src/IceRpc/IConnection.cs

@@ -26,6 +26,11 @@ public interface IConnection
    /// <param name="request">The outgoing request being sent.</param>
    /// <param name="cancel">The cancellation token.</param>
    /// <returns>The corresponding <see cref="IncomingResponse"/>.</returns>
+    /// <exception cref="ConnectionCanceledException">Thrown if the connection was canceled.</exception>


I find the terminology for "a connection being cancelled" weird. I'm not sure users will understand what this means.

pepone

The GenericHost sample needs fixing

src/IceRpc/ClientConnection.cs

pepone · 2022-06-23T07:10:29Z

src/IceRpc/ClientConnection.cs

+        catch (OperationCanceledException exception) when (exception.CancellationToken != cancel)
+        {
+            // exception comes from another call to ConnectAsync
+            throw new ConnectionCanceledException();


I don't quite understand why we need this exception, the core never handles this concrete type

The retry interceptor probably need to be fixed to handle it.

We need an exception to report that the first/main ConnectAsync failed with an OperationCanceledException and left the connection in an unusable state.

For ConnectAsync, we could abort the connection (in the first failed ConnectAsync) and throw ConnectionClosedException here. For ShutdownAsync, it's not as clear, in particular, I think it would be weird for ShutdownAsync to throw ConnectionClosedException in this situation.

I'd rather leave the fixing of the retry interceptor (if any) for later until we figure out what we want to do here.

src/IceRpc/ClientConnection.cs

bentoi

About the ClientConnection.ConnectAsync semantics... It can be called from multiple threads. Shouldn't the cancellation of any of the pending ConnectAsync trigger the connect cancellation? I'm not mistaken, on the fix ConnectAsync call will trigger the connect cancellation. It's the same for ShutdownAsync.

IMO, any canceled connect or shutdown should leave the connection is a state where the only operation that can be called is DisposeAsync. Calling any other operations should raise ConnectionCanceledException.

There's also the issue that we now raise TimeoutException from ConnectAsync the retry interceptor can no longer distinguish the possibly non-retryable invocation timeout or the always retryable connect. Or is the idea to not support retrying invocation timeouts with the retry interceptor?

bentoi · 2022-06-23T08:25:34Z

src/IceRpc/ClientConnection.cs

+        catch (OperationCanceledException exception) when (exception.CancellationToken != cancel)
+        {
+            // exception comes from another call to ConnectAsync
+            throw new ConnectionCanceledException();


The retry interceptor probably need to be fixed to handle it.

src/IceRpc/ClientConnection.cs

src/IceRpc/Internal/ServerConnection.cs

src/IceRpc/Server.cs

src/IceRpc/ClientConnection.cs

pepone · 2022-06-23T09:59:41Z

There's also the issue that we now raise TimeoutException from ConnectAsync the retry interceptor can no longer distinguish the possibly non-retryable invocation timeout or the always retryable connect. Or is the idea to not support retrying invocation timeouts with the retry interceptor?

I think the rule should be to not retry after we exceed the request deadline, if a connect operation timeouts we should still be able to retry, so it is important that we can distinguish between the request deadline expired and other timeouts.

Co-authored-by: Joe George <joe@externl.com>

bernardnormier · 2022-06-23T15:49:25Z

About the ClientConnection.ConnectAsync semantics... It can be called from multiple threads. Shouldn't the cancellation of any of the pending ConnectAsync trigger the connect cancellation?

No, It's only the first or "main" ConnectAsync call that matters. If it's canceled, then the connection is canceled and it becomes unusable forever.
Concurrent ConnectAsync calls wait for this main ConnectAsync call to complete. If they timeout while waiting, they don't cancel the main ConnectAsync. Their demise does not mark the connection as canceled, and the main ConnectAsync can still complete successfully.

I'm not mistaken, on the fix ConnectAsync call will trigger the connect cancellation. It's the same for ShutdownAsync.

Yes.

IMO, any canceled connect or shutdown should leave the connection is a state where the only operation that can be called is DisposeAsync. Calling any other operations should raise ConnectionCanceledException.

That's not the way I see it.

bernardnormier · 2022-06-23T16:02:31Z

There's also the issue that we now raise TimeoutException from ConnectAsync the retry interceptor can no longer distinguish the possibly non-retryable invocation timeout or the always retryable connect. Or is the idea to not support retrying invocation timeouts with the retry interceptor?

I think the rule should be to not retry after we exceed the request deadline, if a connect operation timeouts we should still be able to retry, so it is important that we can distinguish between the request deadline expired and other timeouts.

The solution is obvious here: you install your deadline interceptor before your retry interceptor. Then your retry interceptor won't see Timeout exceptions thrown by your deadline interceptor.

bernardnormier · 2022-06-23T16:12:07Z

As far as retrying on connect Timeout exceptions, the retry interceptor always handles this properly. It's going to retry on ANY exception if the request is idempotent or its payload was not read yet:

                    catch (Exception ex)
                    {
                        response = null;
                        exception = ex;

                        // ConnectionClosedException is a graceful connection closure that is always safe to retry.
                        if (ex is ConnectionClosedException ||
                            request.Fields.ContainsKey(RequestFieldKey.Idempotent) ||
                            !decorator.IsRead)
                        {
                            retryPolicy = RetryPolicy.Immediately;
                        }
                    }

ClientConnection.InvokeAsync "connects" (and throws TimeoutException if ConnectAsync times out) before reading the payload.

pepone · 2022-06-23T16:41:11Z

src/IceRpc/ClientConnection.cs

    /// <exception cref="ConnectionClosedException">Thrown if the connection is already closed.</exception>
+    /// <exception cref="OperationCanceledException">Thrown if cancellation was requested through the cancellation
+    /// token.</exception>
    public async Task ConnectAsync(CancellationToken cancel = default)


Shouldn't the implementation create a cancelation token source with _connectTimeout as we do in InvokeAsync, I don't see why this is only done when ConnectAsync is called from InvokeAsync.

No, the idea is if you call ConnectAsync or ShutdownAsync directly, you pass a cancellation token and decide if/when this call times out through this cancellation token.

This doesn't feel right, if you create a connection and passed a ConnectTimeout it should apply when you call ConnectAsync

No, see #1417 point 1.

Ok, this means the connection pool is not using connection timeouts anymore.

We need to revisit ConnectionPool in a follow-up PR.

examples/GenericHost/Server/Program.cs

src/IceRpc/ClientConnection.cs

externl

Looks good to me!

…t-async

bernardnormier added 17 commits June 21, 2022 14:47

Refactor ClientConnection.ConnectAsync

d88b96a

Checkpoint

a0c84c6

Checkpoint

555ae38

Checkpoint

1f96479

Checkpoint

89ac474

Simplify connection pool

aa5e1f8

Simplify Server and ServerConnection

35c8635

Cleanup

9584159

Cleanup

93db4ca

Cleanup

37f1c29

Cleanup

0ef9df9

Cleanup

dba186e

Cleanup

204d79f

Cleanup

f5d7db5

Cleanup

82e674e

Cleanup

1a4fa27

Merge branch 'main' of github.com:zeroc-ice/icerpc-csharp into connec…

e6ceb57

…t-async

bernardnormier requested review from pepone, externl and bentoi June 22, 2022 19:32

externl reviewed Jun 22, 2022

View reviewed changes

bernardnormier added 4 commits June 22, 2022 17:20

DisposeAsync fix

49a3d81

Simplify ClientConnection

c19bf7f

Remove _isDisposed from Server and ConnectionPool

2382f40

Bug fix

1817093

pepone reviewed Jun 23, 2022

View reviewed changes

bentoi reviewed Jun 23, 2022

View reviewed changes

bernardnormier and others added 2 commits June 23, 2022 10:16

Update src/IceRpc/ConnectionExceptions.cs

6ec1e96

Co-authored-by: Joe George <joe@externl.com>

Update src/IceRpc/ConnectionPool.cs

de4bcef

Co-authored-by: Joe George <joe@externl.com>

bernardnormier and others added 3 commits June 23, 2022 10:17

Update src/IceRpc/ConnectionPool.cs

1eb5182

Co-authored-by: Joe George <joe@externl.com>

Fix some review comments

cf06071

More fixes

ada754e

Fix examples

ebe9ac0

pepone reviewed Jun 23, 2022

View reviewed changes

bernardnormier requested review from pepone, bentoi and externl June 23, 2022 16:58

bernardnormier added 2 commits June 23, 2022 14:22

Remove cancelDispatches and abortInvocations parameters

a6064f3

DisposeAsync fix

90fda7e

bentoi approved these changes Jun 24, 2022

View reviewed changes

examples/GenericHost/Server/Program.cs Outdated Show resolved Hide resolved

src/IceRpc/ClientConnection.cs Outdated Show resolved Hide resolved

pepone approved these changes Jun 24, 2022

View reviewed changes

externl approved these changes Jun 24, 2022

View reviewed changes

bernardnormier added 2 commits June 24, 2022 11:15

Fix review comments

e33f407

Merge branch 'main' of github.com:zeroc-ice/icerpc-csharp into connec…

ae4dfbe

…t-async

bernardnormier merged commit 61f7302 into icerpc:main Jun 24, 2022

bernardnormier mentioned this pull request Jun 24, 2022

ConnectionCanceledException #1441

Closed

bernardnormier deleted the connect-async branch August 25, 2022 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ConnectAsync and ShutdownAsync refactoring #1434

ConnectAsync and ShutdownAsync refactoring #1434

bernardnormier commented Jun 22, 2022

externl Jun 22, 2022

pepone left a comment

pepone Jun 23, 2022

bentoi Jun 23, 2022

bernardnormier Jun 23, 2022 •

edited

Loading

bentoi left a comment

bentoi Jun 23, 2022

pepone commented Jun 23, 2022

bernardnormier commented Jun 23, 2022 •

edited

Loading

bernardnormier commented Jun 23, 2022

bernardnormier commented Jun 23, 2022

pepone Jun 23, 2022 •

edited

Loading

bernardnormier Jun 23, 2022

pepone Jun 23, 2022

bernardnormier Jun 23, 2022

pepone Jun 24, 2022

bernardnormier Jun 24, 2022

externl left a comment

ConnectAsync and ShutdownAsync refactoring #1434

ConnectAsync and ShutdownAsync refactoring #1434

Conversation

bernardnormier commented Jun 22, 2022

Choose a reason for hiding this comment

pepone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

bentoi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone commented Jun 23, 2022

bernardnormier commented Jun 23, 2022 • edited Loading

bernardnormier commented Jun 23, 2022

bernardnormier commented Jun 23, 2022

pepone Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl left a comment

Choose a reason for hiding this comment

bernardnormier Jun 23, 2022 •

edited

Loading

bernardnormier commented Jun 23, 2022 •

edited

Loading

pepone Jun 23, 2022 •

edited

Loading