Rewrite WebSocket class

[alexandrebouthinon]

What does this PR do ?

The WebSocket protocol class has been rewritten.
Before this PR a new SDK instance was create like this:

KuzzleSdk.Kuzzle kuzzle = new KuzzleSdk.Kuzzle(new WebSocket("kuzzle"));
await kuzzle.ConnectAsync();

now this can be done like this:

KuzzleSdk.Kuzzle kuzzle = new KuzzleSdk.Kuzzle(new WebSocket(new Uri("ws://kuzzle:7512")));
kuzzle.ConnectAsync(CancellationToken.None).Wait();

How should this be manually tested?

Run unit tests

Other changes

• Add unit tests.
• Fix documentation tests template.

[benoitvidis]

I agree that passing a Uri instance to the WebSocket protocol is simpler than splitting it between a host string and a set of options but this means we will end-up with totally different protocol constructors per language, which, IMO, is not a problem per-se.

The use of a mandatory CancellationToken on the ConnectAsync method is more problematic to me.
As the end-user is unable to act on the token, the only possible action for him is to generate a self-expiring token, which was previously handled within the default constructor options.

I prefer this last option. Additionally, we could extend it to allow the user to declare his own ws client buffer sizes for instance.

So, to sum up, I would remove the CancellationToken parameter from the ConnectAsync methods and generate one based on the options given to the protocol constructor instead.

[benoitvidis]

Is the <method>.Wait() syntax an alternative to await? And if so, is there a reason to prefer it over await?

[alexandrebouthinon]

.Wait() is a Thread method to wait for the thread termination. I used it here because main function cannot be async.

[Aschen]

Main function does not need to be async, this script is run with dotnet script which allow await keyword in the script root without encapsulating the code in a main async function

[alexandrebouthinon]

Since it is an example of code, should we assume that it will be used in a main function?

[benoitvidis]

Shouldn't we keep a default timeout?

[alexandrebouthinon]

see my comment below 🙂

[alexandrebouthinon]

@benoitvidis:

I agree that passing a Uri instance to the WebSocket protocol is simpler than splitting it between a host string and a set of options but this means we will end-up with totally different protocol constructors per language, which, IMO, is not a problem per-se.

The use of a mandatory CancellationToken on the ConnectAsync method is more problematic to me.
As the end-user is unable to act on the token, the only possible action for him is to generate a self-expiring token, which was previously handled within the default constructor options.

I prefer this last option. Additionally, we could extend it to allow the user to declare his own ws client buffer sizes for instance.

So, to sum up, I would remove the CancellationToken parameter from the ConnectAsync methods and generate one based on the options given to the protocol constructor instead.
I agree that passing a Uri instance to the WebSocket protocol is simpler than splitting it between a host string and a set of options but this means we will end-up with totally different protocol constructors per language, which, IMO, is not a problem per-se.

The use of a mandatory CancellationToken on the ConnectAsync method is more problematic to me.
As the end-user is unable to act on the token, the only possible action for him is to generate a self-expiring token, which was previously handled within the default constructor options.

I prefer this last option. Additionally, we could extend it to allow the user to declare his own ws client buffer sizes for instance.

So, to sum up, I would remove the CancellationToken parameter from the ConnectAsync methods and generate one based on the options given to the protocol constructor instead.

After a discuss with @scottinet, this is ugly but it seems to be a C# idiomatic way to pass optional params (in fact, nothing is optional, all is explicitly given). In addition, this optimization of the performance of the Websocket class is expected and it is an alpha version. We can change the behavior of this function later. 🙂

[benoitvidis]

@alexandrebouthinon , @scottinet , I am sorry, I do not understand the response.

Do you mean c# discourages the use of default parameters? The only reference I found only encourages using method overloads over default parameters.
To me, whether the connection options are given to the constructor or the connect method are totally driven by design, not by the language.

To get the expected default timeout behavior with this proposed implementation, the developer must write this kind of code:

WebSocket socket = new KuzzleSdk.Protocol.WebSocket(new Uri("wss://localhost:7512"));
var kuzzle = new KuzzleSdk.Kuzzle(socket);

var source = new CancellationTokenSource(30000);
await kuzzle.ConnectAsync(source.Token);

This creates yet some other extra verbosity and make It quite easy to forget / skip the CancellationTokenSource creation imo.

Further, what about the auto-reconnect? Which CancellationToken is used then?

About other options

ClientWebSocket supports several other options, including the ability to set some additional http headers or to set the size of the request/answer buffers.

I believe we should give the ability to the developer to access/modify these properties.

About signature and alpha versions

My understanding is that this version is meant to be distributed and used quite soon by a sample of users. Any further breaking change will imply some rework for them.
I believe we should try as hard as we can to define some solid signatures from the start.

[softlair38]

@benoitvidis : I am agree that CancellationToken must be manage by the library and not pass in the constructor. When call Close, it must call Cancel() in cancellationToken.

Default timeout can be 30 but it need to be modified, like the buffer size.

Methods with await, could terminate with .ConfigureAwait(false);

You can use ConcurrentQueue instead of BlockingCollection to keep the order of message

[Shiranuit]

@softlair38

You can use ConcurrentQueue instead of BlockingCollection to keep the order of message

The default collection type for BlockingCollection<T> is ConcurrentQueue<T>, so no need to change it.

[softlair38]

@Shiranuit yes, you are right, I did not use this class until now, thank you

[scottinet]

@benoitvidis > I'm afraid the reasons I gave to Alexandre were a bit lost in translation.
What I said was that it was idiomatic to C#, not because it's ugly or because C# encourages explicit paramters (it does not), but because C# API makes sure that it's obvious that new threads are created when invoking such methods.
It's a lot safer for maintenability reason to pass an explicit CancellationToken.None option when explicitly forfeiting any control to the created thread, instead of having default values that may hide the fact that a new thread is started.

And I think that we should do the same.