Don't invoke on connection before _connectTask has completed successfully

[bernardnormier]

We currently have this odd logic in ProtocolConnection.InvokeAsync, where when IsServer, we wait for the connect task to complete before performing the invocation. That's really a work-around for rare race condition where we accept a dispatch on an incoming connection before its connect task has completed.

Once we eliminate ProtocolConnection, we could for example await _connectTask at the beginning of _acceptRequeststTask in IceRpcProtocolConnection.

See #2241.

[bernardnormier]

This behavior - allowing dispatches on server connection before ConnectAsync has completed - is problematic for another reason.

The (revised) contract for IProtocolConnection is you can't call ShutdownAsync until after ConnectAsync has returned successfully.

Now take Server: it wants to call ShutdownAsync on any connection it accepted where ConnectAsync succeeded OR that has already accept one or more dispatches! And unfortunately a connection can have accepted one or more dispatches before ConnectAsync completion.

[bernardnormier]

we could for example await _connectTask at the beginning of _acceptRequeststTask in IceRpcProtocolConnection.

This does not work: ConnectAsync could still complete after some dispatch was accepted.

I am wondering if we need to add a separate method on IProtocolConnection to "start accepting requests" once the connection is connected. For example:

public interface IProtocolConnection
{
    // Starts accepting requests and dispatching them to dispatcher.
    // Can only be called on a connection after it was successfully connected.
    void Listen(IDispatcher dispatcher);
}

This way we can guarantee no request is accepted until after ConnectAsync completed successfully.

[pepone]

This does not work: ConnectAsync could still complete after some dispatch was accepted.

What is the problem here, we might want to await ConnectAsync And then call shutdown, and in this case we don't want to start accepting requests?

[bentoi]

It's not clear to me if the original issue (#1675) can still occur.

Isn't _connectTask guaranteed to always be assigned now when InvokeAsync is called from a dispatch (executed before _connectTask fully completes)?

[bernardnormier]

For Jose:
ShutdownAsync now throws InvalidOperationException when called before ConnectAsync has completed successfully
https://github.com/zeroc-ice/icerpc-csharp/blob/f6c5318d8f8ce9e61b8728eea77dfa19471da126/src/IceRpc/Internal/ProtocolConnection.cs#L266

I think this makes sense. And it helps with the LogProtocolConnectionDecorator implementation.

we might want to await ConnectAsync And then call shutdown, and in this case we don't want to start accepting requests?

Currently you can accept & dispatch requests while ConnectAsync is still running.
So:

• in Server.ShutdownAsync, you want to call connection.ShutdownAsync on any fully connected connection (easy)
• but if your Server is shutting down while some connection.ConnectAsync is still outstanding. What do you then with that connection? You need to call ShutdownAsync on it just in case it already dispatched a request, which is pretty odd. But you can't because you need to wait for ConnectAsync to complete to avoid the InvalidOperationException.

[bernardnormier]

For Benoit:
the issue is not and never was _connectTask being assigned. It's about _connectTask being completed successfully.

From ProtocolConnection's point of view, the caller must ensure InvokeAsync is called only after ConnectAsync completed successfully. Else InvalidOperationException.

[bentoi]

Now take Server: it wants to call ShutdownAsync on any connection it accepted where ConnectAsync succeeded OR that has already accept one or more dispatches! And unfortunately a connection can have accepted one or more dispatches before ConnectAsync completion.

It's not clear to me what you mean here. No matter what, a connection will never dispatches any requests if ConnectAsync doesn't return successfully.

[bernardnormier]

No matter what, a connection will never dispatches any requests if ConnectAsync doesn't return successfully.

You're missing the point. The question is: can a connection dispatch a request before its ConnectAsync has returned successfully.

The answer is currently yes, which is surprising and problematic for several reasons:

• since a dispatch can go through, it's possible for code in the Server to call InvokeAsync on this connection before ConnectAsync has completed
• when Server shuts down while some connections are connecting at the RPC level, Server is in a conundrum:
  • it would rather just DisposeAsync these connections that are not connected from its point of view
  • but since they may have already dispatched some requests, it should really shut them down
  • but it can't shut them down before ConnectAsync has completed--otherwise it would get an InvalidOperationException

Proposed fix: #2380.

[pepone]

Can we just pass the server shutdown cancelation token to connect async, and check if cancelation has been requested just before we start the dispatch loop?

[bernardnormier]

The issue is not canceling a dispatch on a connection where ConnectAsync has not returned yet.

We need to call ShutdownAsync on such a connection. ShutdownAsync does not cancel dispatches blindly: it stops accepting new requests, sends a GoAway frame, and waits for while before canceling dispatches:

https://github.com/zeroc-ice/icerpc-csharp/blob/363ba89fcdb2a28638b67c520025628f6996fae4/src/IceRpc/Internal/ProtocolConnection.cs#L408