Add a new thread for message sending due to web socket requirements

[lyonsil]

This change is 

[katherinejensen00]

I think this looks pretty good and makes sense.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

c-sharp/MessageTransports/PapiClient.cs line 130 at r1 (raw file):

    /// If false is returned, then there is no need to call <see cref="DisconnectAsync"/> before calling <see cref="Dispose"/>.
    /// </summary>
    /// <returns><see cref="Task"/> will resolve to true if the connection initialied properly, false otherwise</returns>

Suspected Typo: intialied should probably be initialized

[lyonsil]

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @katherinejensen00)

---

c-sharp/MessageTransports/PapiClient.cs line 130 at r1 (raw file):

Previously, katherinejensen00 wrote…

Suspected Typo: intialied should probably be initialized

Fixed - Indeed, your are correct!

[lyonsil]

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @katherinejensen00)

---

c-sharp/MessageTransports/PapiClient.cs line 130 at r1 (raw file):

Previously, lyonsil wrote…

Fixed - Indeed, your are correct!

"you" are correct - man, spelling error when talking about my other spelling error! 😄

[katherinejensen00]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

c-sharp/MessageTransports/PapiClient.cs line 130 at r1 (raw file):

Previously, lyonsil wrote…

"you" are correct - man, spelling error when talking about my other spelling error! 😄

Ha ha. Thanks for making that nitpicky change :)

[FoolRunning]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lyonsil)

---

c-sharp/MessageTransports/PapiClient.cs line 189 at r1 (raw file):

            _incomingMessageThread.Join();
        if (_outgoingMessageThread.IsAlive)
            _outgoingMessageThread.Join();

This looks/feels like the code in DisconnectAsync. Should this also have a timeout or maybe the code of both places be merged together?

---

c-sharp/MessageTransports/PapiClient.cs line 325 at r1 (raw file):

                    }
                    else
                    {

Is there a reason you moved this null check into where the new thread is handling it instead of not starting a new thread in the first place?

[lyonsil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @FoolRunning)

---

c-sharp/MessageTransports/PapiClient.cs line 189 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

This looks/feels like the code in DisconnectAsync. Should this also have a timeout or maybe the code of both places be merged together?

The only use of this function at least right now is for the main program thread to block (in a multi-threading friendly way) once it has done everything else it thinks it needs to do. I could add the timeout back, but it's also a little weird because a timeout passed in will either be cut in half for each thread or be doubled as it applies to each thread.

In DisconnectAsync I definitely wanted a timeout for the reasons we discussed in that PR. For this function, the need for a timeout wasn't as clear to me. It's not hard to add one back here, but combining that with the DisconnectAsync case is a little more complex because when disconnecting it definitely is an error if the threads don't join quickly. In the main program thread case, maybe someone later wants to block here but wake up every X seconds to do something else in the background. If that's the use case, then we don't want to log an error message if the threads didn't join before the timeout happens.

Of course we could have some argument like an optional error message if the timeout happens without joining, but that starts to looks like a clunky function IMO.

I don't have a super string opinion here. If you think they should be combined, please suggest what you think an appropriate function signature for this should be that takes into account:

• Doubling/halving the timeout period,
• Whether to log an error or not if the timeout is reached (including whether the caller should provide the error message)
• If we're looking for a general "block" function, whether we should support a cancellation token like we use in other parts of the code

---

c-sharp/MessageTransports/PapiClient.cs line 325 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Is there a reason you moved this null check into where the new thread is handling it instead of not starting a new thread in the first place?

It could go in either place. I moved it because semantically as I was trying to clarify what each thread was responsible for, I was settling on:

• Outgoing thread is responsible for pulling messages off the blocking queue and passing them to the web socket
• Incoming thread is responsible for pulling data off the web socket, transforming it to a message, and passing it to a worker thread
• Worker threads are responsible for doing whatever is appropriate with a message

I think the resulting code looks a little more balanced between the incoming and outgoing thread functions. Having said that, it really would work in either place.

[lyonsil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @FoolRunning and @katherinejensen00)

---

c-sharp/MessageTransports/PapiClient.cs line 130 at r1 (raw file):

Previously, katherinejensen00 wrote…

Ha ha. Thanks for making that nitpicky change :)

Done.

[FoolRunning]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @katherinejensen00)

---

c-sharp/MessageTransports/PapiClient.cs line 189 at r1 (raw file):

Previously, lyonsil wrote…

The only use of this function at least right now is for the main program thread to block (in a multi-threading friendly way) once it has done everything else it thinks it needs to do. I could add the timeout back, but it's also a little weird because a timeout passed in will either be cut in half for each thread or be doubled as it applies to each thread.

In DisconnectAsync I definitely wanted a timeout for the reasons we discussed in that PR. For this function, the need for a timeout wasn't as clear to me. It's not hard to add one back here, but combining that with the DisconnectAsync case is a little more complex because when disconnecting it definitely is an error if the threads don't join quickly. In the main program thread case, maybe someone later wants to block here but wake up every X seconds to do something else in the background. If that's the use case, then we don't want to log an error message if the threads didn't join before the timeout happens.

Of course we could have some argument like an optional error message if the timeout happens without joining, but that starts to looks like a clunky function IMO.

I don't have a super string opinion here. If you think they should be combined, please suggest what you think an appropriate function signature for this should be that takes into account:

• Doubling/halving the timeout period,
• Whether to log an error or not if the timeout is reached (including whether the caller should provide the error message)
• If we're looking for a general "block" function, whether we should support a cancellation token like we use in other parts of the code

Let's just leave it for now

---

c-sharp/MessageTransports/PapiClient.cs line 325 at r1 (raw file):

Previously, lyonsil wrote…

It could go in either place. I moved it because semantically as I was trying to clarify what each thread was responsible for, I was settling on:

• Outgoing thread is responsible for pulling messages off the blocking queue and passing them to the web socket
• Incoming thread is responsible for pulling data off the web socket, transforming it to a message, and passing it to a worker thread
• Worker threads are responsible for doing whatever is appropriate with a message

I think the resulting code looks a little more balanced between the incoming and outgoing thread functions. Having said that, it really would work in either place.

I guess it's fine.

[katherinejensen00]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)