change: Make StopConnection explicitly async.

[hallsbyra]

By making StopConnection async we expose the inherently async nature of stopping a connection.

Currently, a Poll task is started in StartConnection and then it is signalled to stop in StopConnection. There are a few issues with this.

1. There is no way for the caller to know when the connection is really closed. StopConnection only signals the intention to close the connection.
2. The connection is closed while the poll thread is running, which can lead to weird problems, e.g. the poll thread writing to a closed connection.
3. Situation 2 can actually lead to the connection being opened up again (see e.g. TcpClientOsdpConnection.WriteAsync()). If so, it will never be closed.

This PR

1. Only closes the connection once the poll thread has exited.
2. Makes StopConnection async to capture the whole process of closing a connection. When the StopConnection task is completed, the connection is guaranteed to be stopped.

Having StopConnection return Task is a "somewhat" breaking change. Old code will compile, but with a warning. To get rid of the warning one can either

// Ignore the task. More or less the old behavior.
_ = cp.StopConnection()

// or

// Await the task (preferred). This way you have explicit control of the shutdown behavior.
await cp.StopConnection()

[hallsbyra]

awaiting your feedback before I proceed to fix compilation warnings! :)

[bytedreamer]

This is a good idea. There are two areas that should be addressed before adding it to the main repo. I worry that starting a new connection would fail while the old one is attempting to close. Not sure if the client would even know if it failed.

• Add a timeout that forces the poll thread to shutdown after a certain amount of time
• The start connection should wait for the close to complete before attempting to start a new polling thread

[hallsbyra]

I'm not sure I follow. Do you mean a client could do something like this:

c1 = cp.StartConnection(con1);
c2 = cp.StartConnection(con1);

I would say that is illegal. One would have to do this:

c1 = cp.StartConnection(con1);
await cp.StopConnection(c1);
c2 = cp.StartConnection(con1);

That should be fine.

[bytedreamer]

I agree, the first one should not be allowed. I believe an Invalid Operation Exception response would be an appropriate response.

The second scenario is the concern I had with the preceding comment. If it is running on the same thread, there should be no issues with the change. There could be an issue when the shutdown is on a different thread from the connection staring back up. While the second the statement is waiting for a shutdown, how will the third statement behave on a different thread? It appears that it will not connect and the client won't get any feedback. Ideally, it would wait for the shutdown to complete and attempt to connect within the timeout. If it easier, it could also return the Exception as described above.

What do you think?

[hallsbyra]

Sorry for the delay - took some Easter leave! :) Anyway, my take on the above is that if it is illegal to start a new connection before the old one is completely closed, then it does not matter whether this happens in a single thread or from multiple threads - it should still be illegal. The rule is: you must ensure a connection is fully stopped before starting it again. This PR enables the client to enforce that rule, by letting it observe the Task and wait for its completion.

I fear that trying to synchronize IOsdpConnections between Buses can be a can of worms. In my view it would be like if you passed the same Stream instance to several StreamReaders and expect that they would somehow sort out how to cooperate on that single Stream.

If we should do anything at all to prevent the same connection from being misused I can think of two pretty simple checks:

1. In StartConnection, ensure that the passed connection instance is not already used by any Bus.
2. In StartConnection, ensure that the passed connection is not already connected.

However, I see problems with both... Apart from thread synchronization issues, it could still come down to semantics. What if two different SerialPortOsdpConnections instances are started, but they use the same serial port (COMx)? Should we try to prevent that as well?

In short, my suggestion is to not try to be oversmart in the ControlPanel. Keep it simple. Don't you agree?

[bytedreamer]

Agreed, throw an exception if the polling task is still running. We should at least let the caller know that it didn't start a new connection.

[hallsbyra]

Ok. I added a check in StartConnection. Let me know what you think.