Pair incoming responses to the original requests (#61)

[lyonsil]

This is a PR on top of Ira's recent Resharpier update. It includes all feedback from the original, draft PR.

---

This change is 

[FoolRunning]

Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson, @lyonsil, and @tjcouch-sil)

---

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

using System.Collections.Concurrent;

Something bad happened here. This was not treated as a renamed file - making it really hard to see what was changed.
Is there anything you can do to make it detect as a rename?

[FoolRunning]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson, @lyonsil, and @tjcouch-sil)

---

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

Previously, FoolRunning (Tim Steenwyk) wrote…

Something bad happened here. This was not treated as a renamed file - making it really hard to see what was changed.
Is there anything you can do to make it detect as a rename?

It may require you to change to two commits (one to rename the file and one to make changes to the file).

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

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

Previously, FoolRunning (Tim Steenwyk) wrote…

It may require you to change to two commits (one to rename the file and one to make changes to the file).

This shows up in git as a rename. Maybe it's just something with Reviewable?

 22 files changed, 541 insertions(+), 374 deletions(-)
 delete mode 100644 c-sharp/Data/NetworkConnectorTypes.cs
 rename c-sharp/{Data => JsonUtils}/EnumConverter.cs (96%)
 rename c-sharp/{Data => JsonUtils}/MessageConverter.cs (90%)
 rename c-sharp/{Data => JsonUtils}/PrivateConstructorResolver.cs (95%)
 rename c-sharp/{Utils/JsonUtils.cs => JsonUtils/SerializationOptions.cs} (84%)
 create mode 100644 c-sharp/MessageHandlers/IMessageHandler.cs
 create mode 100644 c-sharp/MessageHandlers/MessageHandlerEvent.cs
 create mode 100644 c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs
 create mode 100644 c-sharp/MessageHandlers/MessageHandlerResponse.cs
 rename c-sharp/{Data/RequestReturn.cs => MessageHandlers/ResponseToRequest.cs} (58%)
 rename c-sharp/{Web => MessageTransports}/PapiClient.cs (50%)
 create mode 100644 c-sharp/Messages/Message.cs
 create mode 100644 c-sharp/Messages/MessageClientConnect.cs
 create mode 100644 c-sharp/Messages/MessageEvent.cs
 create mode 100644 c-sharp/Messages/MessageInitClient.cs
 create mode 100644 c-sharp/Messages/MessageRequest.cs
 create mode 100644 c-sharp/Messages/MessageResponse.cs
 create mode 100644 c-sharp/Messages/MessageType.cs
 create mode 100644 c-sharp/Messages/NetworkConnectorInfo.cs
 create mode 100644 c-sharp/Messages/README.md
 create mode 100644 c-sharp/Messages/RequestType.cs

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

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

Previously, lyonsil wrote…

This shows up in git as a rename. Maybe it's just something with Reviewable?

 22 files changed, 541 insertions(+), 374 deletions(-)
 delete mode 100644 c-sharp/Data/NetworkConnectorTypes.cs
 rename c-sharp/{Data => JsonUtils}/EnumConverter.cs (96%)
 rename c-sharp/{Data => JsonUtils}/MessageConverter.cs (90%)
 rename c-sharp/{Data => JsonUtils}/PrivateConstructorResolver.cs (95%)
 rename c-sharp/{Utils/JsonUtils.cs => JsonUtils/SerializationOptions.cs} (84%)
 create mode 100644 c-sharp/MessageHandlers/IMessageHandler.cs
 create mode 100644 c-sharp/MessageHandlers/MessageHandlerEvent.cs
 create mode 100644 c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs
 create mode 100644 c-sharp/MessageHandlers/MessageHandlerResponse.cs
 rename c-sharp/{Data/RequestReturn.cs => MessageHandlers/ResponseToRequest.cs} (58%)
 rename c-sharp/{Web => MessageTransports}/PapiClient.cs (50%)
 create mode 100644 c-sharp/Messages/Message.cs
 create mode 100644 c-sharp/Messages/MessageClientConnect.cs
 create mode 100644 c-sharp/Messages/MessageEvent.cs
 create mode 100644 c-sharp/Messages/MessageInitClient.cs
 create mode 100644 c-sharp/Messages/MessageRequest.cs
 create mode 100644 c-sharp/Messages/MessageResponse.cs
 create mode 100644 c-sharp/Messages/MessageType.cs
 create mode 100644 c-sharp/Messages/NetworkConnectorInfo.cs
 create mode 100644 c-sharp/Messages/README.md
 create mode 100644 c-sharp/Messages/RequestType.cs

The change looks like a diff in github, too, so it definitely looks like an issue with Reviewable.

[FoolRunning]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @irahopkinson and @tjcouch-sil)

---

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

Previously, lyonsil wrote…

The change looks like a diff in github, too, so it definitely looks like an issue with Reviewable.

Sure enough. I should have checked that first.

[irahopkinson]

Looking good, just one question.

Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil and @tjcouch-sil)

---

c-sharp/Program.cs line 25 at r1 (raw file):

            return;
        }
        Console.WriteLine("Paranext .NET connected");

FYI: this is currently logged such that we know it is from the dotnet data provider so the extra context that you added is not needed but also not an issue:

---

c-sharp/Program.cs line 41 at r1 (raw file):

        bool result = await connection.RegisterRequestHandler(RequestType.AddOne, RequestAddOne);
        if (!result)
            return;

Sorry I'm not familiar enough with C# threads. What happens to the thread that has been started should we take this early return path? Does it get cleaned up appropriately or does it need to be stopped?

[FoolRunning]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

c-sharp/Program.cs line 41 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Sorry I'm not familiar enough with C# threads. What happens to the thread that has been started should we take this early return path? Does it get cleaned up appropriately or does it need to be stopped?

Good catch. I'm fairly certain that since the thread is not marked as being a background thread and has already been started, the thread will continue to run, keeping the process from exiting until the connection is forcibly closed by the server (which will finally end the thread).

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

c-sharp/Program.cs line 41 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Good catch. I'm fairly certain that since the thread is not marked as being a background thread and has already been started, the thread will continue to run, keeping the process from exiting until the connection is forcibly closed by the server (which will finally end the thread).

The thread was started in Program.cs with the following code:

        Thread messageHandlingThread =
            new(
                new ThreadStart(async () =>
                {
                    await connection.HandleMessages();
                    messageHandlingIsComplete.Set();
                })
            );

Once the lambda function returns that was given to the ThreadStart object, the thread is complete. So in this case, once HandleMessages returns for any reason, we set the event, and the thread complete. As Tim mentioned the thread could have been marked as a background thread which would not prevent process termination even if the thread was still active. That didn't seem necessary here and really might not even be desired.

I should note that I think there is a reasonable chance that who actually owns the thread calling HandleMessages should change in the future. In my experience a more typical pattern with something like PapiClient is that you call Connect() or Start() on the client object, and that client object owns its own threading internally. Then the owner of the client object calls Stop() or Disconnect() which will internally shut down whatever additional threading was done. I was attempting to limit just how much code to change for this PR. I felt I was probably already over the line for what was expected, and there wasn't any urgent need for more significant changes. An additional thread was really needed here for what we are building, but putting it into the client would have caused even more changes to PapiClient.cs which itself was apparently more than Reviewable could handle without treating it as a new file altogether.

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

c-sharp/Program.cs line 41 at r1 (raw file):

Previously, lyonsil wrote…

The thread was started in Program.cs with the following code:

        Thread messageHandlingThread =
            new(
                new ThreadStart(async () =>
                {
                    await connection.HandleMessages();
                    messageHandlingIsComplete.Set();
                })
            );

Once the lambda function returns that was given to the ThreadStart object, the thread is complete. So in this case, once HandleMessages returns for any reason, we set the event, and the thread complete. As Tim mentioned the thread could have been marked as a background thread which would not prevent process termination even if the thread was still active. That didn't seem necessary here and really might not even be desired.

I should note that I think there is a reasonable chance that who actually owns the thread calling HandleMessages should change in the future. In my experience a more typical pattern with something like PapiClient is that you call Connect() or Start() on the client object, and that client object owns its own threading internally. Then the owner of the client object calls Stop() or Disconnect() which will internally shut down whatever additional threading was done. I was attempting to limit just how much code to change for this PR. I felt I was probably already over the line for what was expected, and there wasn't any urgent need for more significant changes. An additional thread was really needed here for what we are building, but putting it into the client would have caused even more changes to PapiClient.cs which itself was apparently more than Reviewable could handle without treating it as a new file altogether.

If you think we should take this a step further, I think what I would propose is something like:

• Move thread ownership into PapiClient, including changes to how it connects/disconnects
• Make PapiClient disposable which will check for live connections and shut them down if Dispose is called.
• Move PapiClient into a using block so it gets disposed automatically whenever Main() returns for any reason.

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

c-sharp/Program.cs line 41 at r1 (raw file):

Previously, lyonsil wrote…

If you think we should take this a step further, I think what I would propose is something like:

• Move thread ownership into PapiClient, including changes to how it connects/disconnects
• Make PapiClient disposable which will check for live connections and shut them down if Dispose is called.
• Move PapiClient into a using block so it gets disposed automatically whenever Main() returns for any reason.

Here is a branch with the changes I mentioned above. I can add this to the PR if you think it's better to include now.
e7c5e81

[irahopkinson]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FoolRunning, @lyonsil, and @tjcouch-sil)

---

c-sharp/Program.cs line 41 at r1 (raw file):

Previously, lyonsil wrote…

Here is a branch with the changes I mentioned above. I can add this to the PR if you think it's better to include now.
e7c5e81

I do like that code better. I was thinking of a similiar change for the TS - as in making papi the main service (so I'm biased). I'd check with @FoolRunning first though since he's the C# architect.

[irahopkinson]

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

---

c-sharp/Program.cs line 41 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I do like that code better. I was thinking of a similiar change for the TS - as in making papi the main service (so I'm biased). I'd check with @FoolRunning first though since he's the C# architect.

Will resolve this in a folow up PR.