Review comments and fixes #30
Conversation
| invocation (send a request and receive the corresponding response) on a client connection or on a server connection | ||
| with exactly the same API. Any connection, client or server, can also accept incoming requests and dispatch these | ||
| requests to your services. | ||
| connection from a client, this connection is called a "server connection". Once a connection is established, there is no difference between a client and a server connection. You can send requests |
There was a problem hiding this comment.
I find the existing version clearer.
There was a problem hiding this comment.
Ok fine.
I should have explained why I proposed another version though. So for the record: this text refers to an unknown API and I don't find it's the right spot to define what is an invocation or dispatch. This makes the documentation more verbose especially since we define these somewhere else.
| @@ -29,8 +25,7 @@ await using var clientConnection = new ClientConnection(new Uri("icerpc://hello. | |||
| ``` | |||
|
|
|||
| `ClientConnection`'s constructor specifies the [address of the server](server-address#client-connection-configuration), | |||
| but does not actually establish the connection. The connection is established later on by an asynchronous call such as | |||
| `ConnectAsync`: | |||
| but does not actually establish the connection. The connection is established later on by a call such as `ConnectAsync`: | |||
There was a problem hiding this comment.
asynchronous is very relevant here:
- the constructor is synchronous and therefore can't establish the connection
- the connection establishment is performed by a separate async call
There was a problem hiding this comment.
Ok but to me It's obvious that ConnectAsync is asynchronous and that a constructor is not going to perform any blocking calls.
There was a problem hiding this comment.
I anticipate it won't be obvious once we support other languages without this Async suffix convention.
The future text will be language-dependent. E.g. if you select Rust, it will use Rust API as examples.
| @@ -32,10 +32,10 @@ server will use TLS. | |||
| The tcp transport may or may not use TLS. If you specify TLS configuration when you create your client connection for | |||
| tcp, the connection will use TLS. If you don't specify TLS configuration, the connection won't use TLS. | |||
|
|
|||
| In C#, this TLS configuration is a `SslClientAuthenticationOptions` parameter. For example: | |||
| In C#, this TLS configuration is provided with a `SslClientAuthenticationOptions` parameter. For example: | |||
| @@ -49,6 +49,8 @@ It's the same for servers using tcp. If you specify TLS configuration when you c | |||
| accept connections secured by TLS. If you don't specify TLS configuration when you create this server, the server will | |||
| only listen for and accept plain tcp connections. | |||
|
|
|||
| BENOIT: I would remove this information. To me, the fact that tcp and ssl are the same transport is an implementation detail. It also gives the impression that a client can either connect with tcp or with ssl if a server specifies SSL configuration. I find that the explanation above is sufficient and clearer. | |||
There was a problem hiding this comment.
I think we should include it. The user can configure transports and need to know tcp and ssl are both implemented by TcpClientTransport / TcpServerTransport.
There was a problem hiding this comment.
That's one thing I find confusing with IceRPC. We document an "ssl" transport but there's no SSL transport. We only have tcp, quic and coloc transports.
A transport might support secure connections (quic), non secure connections (coloc) or both (tcp).
I think it's something we considered at some point and after reading the documentation, I find it would have been clearer to only support real transports for the "transport" parameter and have a "secure=false|true" parameter that the transport is responsible for checking based on its implementation.
There was a problem hiding this comment.
The current setup is better even though it is apparently slightly confusing.
Previous setup was: tcp transport (and no ssl transport) with tls=true|false parameter.
That was not good because whether or not you use tls is not a URI tcp parameter but a local configuration concern. If you pass a non-null SslClient|ServerAuthenticationOptions when you create a client connection resp. server, you get (a) secure connection(s). Better, clearer, not error prone, not exposed in URIs.
Then we added the ssl transport only for interop with Ice. When we receive a Slice1-encoded proxy with a ssl endpoint, we can't decode it as a tcp server address. It would be (a) confusing and (b) lossy and our server address decoding is lossless.
That's why we introduced this ssl transport, which is identical to tcp except it's always secure, meaning:
-
for a client ssl connection, a null SslClientAuthenticationOptions is equivalent to a non-null empty SslClientAuthenticationOptions (we do the same for Quic)
https://github.com/icerpc/icerpc-csharp/blob/5921b1d1ef0d7ab0952f625da7498e27395569f6/src/IceRpc/Transports/TcpClientTransport.cs#L55 -
for a server with a ssl server address, a null SslServerAuthenticationOptions is an error:
https://github.com/icerpc/icerpc-csharp/blob/5921b1d1ef0d7ab0952f625da7498e27395569f6/src/IceRpc/Transports/TcpServerTransport.cs#L48
In particular, with IceRPC, if you establish an ice/tcp connection to an Ice server with a non-null SslClientAuthenticationOptions, you are establishing a ssl connection. tcp and ssl are synonymous except for the behavior described above.
There was a problem hiding this comment.
I'm not convinced that a tls parameter would be more confusing than a pseudo ssl transport. And for sure this parameter would better capture the design of IceRPC transports where a same transport can (eventually) both support secure and un-secure connections.
An SSL endpoint could be mapped to a tcp and "tls=true" server address (and vice-versa). The local validation of the SSL authentication options can be based on the "tls" parameter instead of a pseudo SSL transport name. It's pretty much the same.
There was a problem hiding this comment.
An SSL endpoint could be mapped to a tcp and "tls=true" server address (and vice-versa).
That's not correct.
In particular, ice://host?transport=tcp&tls=false does not mean you're using tcp. You could use ssl. Pretty confusing.
IF we replace the transport name ssl by a tls parameter for tcp (and I don't think we should), the value should not be true|false but maybe|required:
can establish the connection with tcp or ssl:
ice://host?transport=tcp&tls=maybe
can only establish the connection with ssl:
ice://host?transport=tcp&tls=required
There was a problem hiding this comment.
Ok true. It could also be require-tls=true|false or we could just reject the case where tls=false and authentication options are set and consider these two settings incompatible.
| There are 3 common types of invokers: | ||
|
|
||
| - **Leaf invoker**\ | ||
| It's a leaf in the invoker pipeline that implements `invoker` without the help of another invoker. This leaf invoker is typically a connection. |
There was a problem hiding this comment.
I don't understand this sentence.
There was a problem hiding this comment.
It should be:
It's a leaf in the invocation pipeline that implements `invoke` without the help of another invoker. This leaf
invoker is typically a connection or connection cache.
Similar to the "leaf dispatcher" description:
It's a leaf in the dispatch pipeline that implements `dispatch` without the help of another dispatcher. For example,
a Slice service (LINK).
|
|
||
| ## Request payload and payload continuation | ||
|
|
||
| The payload of a request is a stream of bytes that represents the argument(s) of an operation. When a connection sends a | ||
| request, it reads and logically copies these bytes to the network connection until there is no more byte to read. | ||
| The payload of a request is a stream of bytes. It typically represents the argument(s) of an operation. When a |
There was a problem hiding this comment.
This update is not correct.
|
|
||
| On the other side, the connection reads these bytes from the network and give them to a | ||
| [dispatcher](../dispatch/dispatch-pipeline#the-dispatcher-abstraction). | ||
|
|
||
| BENOIT: the continuation sending can continue after the response so this paragraph isn't really correct? |
There was a problem hiding this comment.
I believe the paragraph below is correct.
| @@ -8,6 +8,7 @@ description: Learn about the ice protocol and duplex transports | |||
| When you create a client connection to server address `ice://hello.zeroc.com`, you instruct IceRPC to establish a | |||
| connection that uses the ice protocol. | |||
|
|
|||
| BENOIT: the application layer from the internet protocol suite or OSI model? I wouldn't mention this detail, it doesn't bring very useful information to the reader. | |||
There was a problem hiding this comment.
I think it's useful otherwise I would not have included it.
| @@ -95,6 +95,7 @@ icerpc provides the most direct realization of IceRPC's APIs and features. In pa | |||
| There is currently only one standard multiplexed transport: QUIC. Since QUIC is new and not universally available, you | |||
| may want to use icerpc with a traditional duplex transport such as TCP. | |||
|
|
|||
| BENOIT: why Slic component and not just Slic transport? | |||
There was a problem hiding this comment.
Is Slic actually a transport?
I see it more as an adapter that adapts a duplex transport into a multiplexed transport.
My transport is "slic" does not mean anything. You'd use icerpc://host?transport=quic but never icerpc://host?transport=slic.
There was a problem hiding this comment.
Is Slic actually a transport?
Yes and no 🙂. No because it's more a "transport layer" than a transport (very much like TLS). Yes because it's implemented like any multiplexed transport in C#.
There was a problem hiding this comment.
This actually raises (again) the question of having SlicServerTransport and SlicClientTransport.
An alternative could have been to have public:
SlicOptions,SlicTransportLayer.CreateListener(IDuplexListener listener, MultiplexedConnectionOptions options, SlicOptions options)SlicTranspotLayer.CreateConnection(IDuplexConnection connection, MultiplexedConnectionOptions options, SlicOptions slicOptions)
TcpXxxTransport and ColocXxxTransport would implement both the duplex and multiplexed transport interfaces. They would provide constructors that take TcpTransportOptions and SlicOptions arguments.
This would be more conform with what Slic really is and with the fact that there's no slic transport (transport=slic doesn't work!).
There was a problem hiding this comment.
I believe the current structure and names are fine.
SlicClientTransport implements IMultiplexedClientTransport using a duplex transport. All good.
Does it mean SlicClientTransport is a transport implementation? Not necessarily.
It's like, is the logger interceptor an invoker? It is since it implements the IInvoker interface; but not really since it needs a "next" (or decoratee) to do the job.
There was a problem hiding this comment.
To me it's related to the conversation we have above with SSL.
Today:
- IceRPC supports an "ssl" transport but doesn't provide a transport implementation for it (no
SslServerTransportorSslClientTransportclasses) - IceRPC doesn't support a "slic" transport but does provide a transport implementation for it (
SlicServerTransportandSlicClientTransportclasses).
Isn't it quite confusing 🙂?
Both are transport layers. SSL is a layer to add security on top of a reliable stream oriented transport (what we call a duplex transport in IceRPC). Slic is a layer to provide multiplexed streams on top of a reliable stream oriented transport.
Retrospectively, I find it would have been better for the URI configuration to match the transport API:
- a
tls=true|falseparameter used by the TCP transport classes to figure out wether or not to add the TLS layer. - no Slic transport classes but TCP & Coloc transport classes that implement the duplex + multiplexed interfaces. Like for SSL (with the SslStream decorator), tcp & coloc rely on Slic layering classes to provide the multiplexing.
This way it's simple. No need to talk about ssl or slic transports in our documentation. We only talk about tcp, coloc and quic. We only have APIs for creating Tcp/Coloc/Quic client/server transports.
tcp and coloc use the Slic transport layer to provide multiplexing.
tcp uses the SSL transport layer for secure connections.
There was a problem hiding this comment.
I don't agree with any of the above.
First, Ssl is not a transport layer in IceRPC. (It is one in Ice which I believe was a mistake.) If Ssl was a transport layer, then we could easily create a "secure coloc" connection. Fortunately it's not and we can't.
Then, we had for a while no ssl transport and instead tls=true|false. See my earlier comment that explains why that's not correct.
Finally, I believe our current duplex/multiplexed structure is correct. The tcp transport implements the simple Duplex transport API and does not inherit / implement / concern itself with the much more complicated Multiplexed transport API.
tcp and coloc use the Slic transport layer to provide multiplexing.
We should not do that.
There was a problem hiding this comment.
Ssl is not a transport layer
TLS = "Transport Layer Security" and clearly our TCP implementation considers it as a transport layer since there's a single transport implementation for the TCP transport and the SSL pseudo transport.
If Ssl was a transport layer, then we could easily create a "secure coloc" connection
I don't see why we would do such a thing.
With the current design, the day we want to support a transport and provide the possibility to optionally make it secure, we'll need again two have two transport names although there will still be a single transport implementation.
tcp and coloc use the Slic transport layer to provide multiplexing.
We should not do that.
It would be a lot clearer.
Today, you don't configure a "slic" transport in the URI but you configure a Slic transport in the code.
Today, you configure a "ssl" transport in the URI but you don't configure a Ssl transport in the code.
Anyway, it doesn't look like I'll be able to convince you so let's keep what we have now.
This is first batch of comments and fix proposals. I prefer to submit a PR over creating lots of small issues but if you prefer I can submit issues... Depending on the PR review, I'll remove/fix the comments.