Send/receive events and add unit tests (#58)

[lyonsil]

This change is 

[irahopkinson]

I started reviewing this and then realized that the base branch had already been merged (and deleted) and so I thought the diff would be wrong but for some reason Reviewable is still showing the correct diff to the orignal base that is now gone. Well done *Reviewable! 👏

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @lyonsil)

---

.vscode/extensions.json line 9 at r1 (raw file):

    "mrmlnc.vscode-json5",
    "streetsidesoftware.code-spell-checker",
    "formulahendry.dotnet-test-explorer"

nit: prefer alphabetical order where possible to help with finding items.

---

.vscode/extensions.json line 9 at r1 (raw file):

    "mrmlnc.vscode-json5",
    "streetsidesoftware.code-spell-checker",
    "formulahendry.dotnet-test-explorer"

Thanks for adding this to the list. I already had it installed so can recommend it also.

---

.vscode/settings.json line 2 at r1 (raw file):

{
  "dotnet-test-explorer.testProjectPath": "c-sharp-tests/c-sharp-tests.csproj",

nit: prefer alphabetical order where possible to help with finding items.

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("c-sharp-tests")]

I'm guessing the tests can't live in c-sharp/tests?

---

c-sharp/README.md line 1 at r1 (raw file):

# Helpful Tips

Thanks for adding this README. Nice job and the links make it easy too.

---

c-sharp/JsonUtils/MessageConverter.cs line 86 at r1 (raw file):

        // Find the right message type to deserialize
        var messageType = ReadValue<MessageType>(ref readerClone, "type");

I'm not sure I agree with changing to use var here since it isn't obvious it will return an Enum, thoughts?

---

c-sharp/JsonUtils/MessageConverter.cs line 93 at r1 (raw file):

        if (messageDataType == typeof(MessageEvent))
        {
            readerClone = reader; // Ordering of items in JSON isn't guaranteed

nit: comments on their own line, see our guide. Here and all over the place.

---

c-sharp/JsonUtils/MessageConverter.cs line 102 at r1 (raw file):

        var msg = (Message)
            JsonSerializer.Deserialize(ref reader, messageDataType!, s_recursiveSafeOptions)!;

Can you still assert messageDataType is not null with the addition of L95-96? Above we throw if the TryGetValue fails but not here.

---

c-sharp/Messages/EventType.cs line 7 at r1 (raw file):

public sealed class EventType : EnumType
{
    public static readonly Enum<EventType> ClientConnect = new("network:onDidClientConnect");

These 3 look like they are linking to TS items. If that's so can you add comments with where they came from?

---

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

                        _ = Task.Run(() =>
                        {
                            HandleMessage(message);

Thanks for breaking this up in to more manageble and readable methods.

---

c-sharp-tests/c-sharp-tests.csproj line 1 at r1 (raw file):

<Project Sdk="Microsoft.NET.Sdk">

Thanks for adding the tests. We should add them to GHA in .github\workflows\test.yml so they run on each PR. Are you comfortable to do that? Either you or I could do that in a follow-up PR but it would be cool to see them on this PR.

---

c-sharp-tests/c-sharp-tests.csproj line 15 at r1 (raw file):

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.3.2" />
    <PackageReference Include="xunit" Version="2.4.2" />

Any reason for preference of xunit over nunit?

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 7 at r1 (raw file):

namespace TestParanextDataProvider.MessageHandlers;

public class MessageConverterTest

I'd feel happier if the file was named the same as this class. A couple of times during review I've opened this file and been confused that it didn't match what I was seeing in Reviewable since this test file is named the same as the class file it's testing.

nit: I'd name this class in plural, i.e. MessageConverterTests.

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 49 at r1 (raw file):

    [Fact]
    public void TestMessageEventThatDoesNotExist()

I believe it's a somewhat typical convention to name unit tests in 3 parts separated by underscores: first part is the method name that is being tested (or SUT); second part is any special or particular setup; third part (optional) is expected outcomes. So this test method could be MessageEvent_DoesNotExist(). Given that, your names are pretty closely matching this.

Perhaps this is a style concern we should discuss together?

---

c-sharp-tests/MessageHandlers/MessageHandlerEvent.cs line 6 at r1 (raw file):

namespace TestParanextDataProvider.MessageHandlers;

public class MessageHandlerEventTest

I'd feel happier if the file was named the same as this class.

[FoolRunning]

Reviewed 24 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("c-sharp-tests")]

That reminds me that we at some point, not part of this PR, need to create the assembly info correctly with the correct copyright, etc.

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'm guessing the tests can't live in c-sharp/tests?

Ideally not since it makes it more complicated to create a release build without the test code included in the built DLL.

---

c-sharp/JsonUtils/MessageConverter.cs line 51 at r1 (raw file):

        {
            var baseType = obj!.GetType().BaseType;
            while (baseType != null)

What is the purpose of these loops? It appears to just look at the base types of each object looking for a non-abstract base type, but in the possibilities list above you already ensure that the types in the list are not abstract. Is the problem that there are some non-abstract classes that are used as a base type?

---

c-sharp/JsonUtils/MessageConverter.cs line 86 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'm not sure I agree with changing to use var here since it isn't obvious it will return an Enum, thoughts?

Per our guidelines, var should only be used if it's obvious. Here it is not, so var should not be used.

---

c-sharp/JsonUtils/MessageConverter.cs line 98 at r1 (raw file):

                messageDataType = eventMessageDataType;

            readerClone = reader; // We'll need it again below

I think it's dangerous to not keep the cloning of the state near where the reading happens (i.e. down where GetEventData is called). The cloning needs to happen before any call to ReadValue. It might be best to instead have the cloning happen inside of ReadValue so callers don't have to worry about it.

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 22 at r1 (raw file):

    private readonly ConcurrentDictionary<Enum<EventType>, PapiEventHandler> _handlers = new();

    public void RegisterEventHandler(Enum<EventType> eventType, PapiEventHandler handler)

I'm not convinced that we should allow more than one handler for a specific event type. Only allowing one would greatly simplify things, I think.

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 70 at r1 (raw file):

        List<Message> messages = new();
        MessageEvent messageEvent = (MessageEvent)message;
        if (_handlers.TryGetValue(messageEvent.EventType, out var handlersForEventType))

Without locking here, you can get into the situation where a Register or Unregister is half-run (i.e. between the TryGetValue and the setting of the new delegate). It won't necessarily cause problems, but it would still be best if a lock was consistently used (and it changed to be a normal dictionary). It could also keep potential problems happening in the future if more code is added to Register/Unregister.
The lock only needs to cover the TryGetValue call so it shouldn't cause any problems (although it will probably make the code a little awkward).

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 63 at r1 (raw file):

            );
        }
        return messageList;

If we decide to keep multiple message handlers (I'm hoping not), this should be changed to yield return calls instead of creating a new list all the time containing one item.

---

c-sharp/Messages/MessageEvent.cs line 16 at r1 (raw file):

    {
        // Default for new events that don't have a custom class
        EventContentsType = typeof(System.Text.Json.JsonElement);

usings should be used instead of using fully qualified names.

---

c-sharp/Messages/MessageEventClientConnect.cs line 10 at r1 (raw file):

    /// </summary>
    private MessageEventClientConnect()
        : base(Messages.EventType.ClientConnect) { }

usings should be used instead of using fully qualified names.

---

c-sharp/Messages/MessageEventClientConnect.cs line 13 at r1 (raw file):

    public MessageEventClientConnect(MessageEventClientConnectContents eventContents)
        : base(Messages.EventType.ClientConnect, eventContents) { }

usings should be used instead of using fully qualified names.

---

c-sharp/Messages/MessageEventClientDisconnect.cs line 13 at r1 (raw file):

    public MessageEventClientDisconnect(MessageEventClientDisconnectContents eventContents)
        : base(Messages.EventType.ClientDisconnect, eventContents) { }

usings should be used instead of using fully qualified names.

---

c-sharp/Messages/MessageEventNetworkObject.cs line 12 at r1 (raw file):

    public MessageEventObjectDispose(string eventContents)
        : base(Messages.EventType.ObjectDispose, eventContents) { }

usings should be used instead of using fully qualified names.

---

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

                    {
                        // Handle each message asynchronously so we can keep receiving more messages
                        _ = Task.Run(() =>

I'm fairly certain you can remove the _ = from this line.

---

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

                    foreach (var m in messagesToSend)
                    {
                        await SendMessageAsync(m, _cancellationTokenSource.Token);

I think the answer is that we're fine, but since this is run asynchronously, we'll be sending data while we might be receiving data on another thread. Is that a problem?

---

c-sharp-tests/Usings.cs line 1 at r1 (raw file):

global using Xunit;

I did not realize you could do this. Fascinating.

I've never used XUnit. We probably need to discuss as a group the best C# testing framework to use (SIL usually uses NUnit).

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 49 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I believe it's a somewhat typical convention to name unit tests in 3 parts separated by underscores: first part is the method name that is being tested (or SUT); second part is any special or particular setup; third part (optional) is expected outcomes. So this test method could be MessageEvent_DoesNotExist(). Given that, your names are pretty closely matching this.

Perhaps this is a style concern we should discuss together?

It's part of Microsoft's best practices (I suggest we adopt the testing conventions as well).

[FoolRunning]

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Ideally not since it makes it more complicated to create a release build without the test code included in the built DLL.

Ignore my previous comment. I read it wrong.

[lyonsil]

Reviewable status: 28 of 29 files reviewed, 20 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

.vscode/extensions.json line 9 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

nit: prefer alphabetical order where possible to help with finding items.

Done - yes, good idea.

---

.vscode/settings.json line 2 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

nit: prefer alphabetical order where possible to help with finding items.

Isn't it already in alphabetical order here?

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

That reminds me that we at some point, not part of this PR, need to create the assembly info correctly with the correct copyright, etc.

Do you want to create an issue in the backlog with the details of what you think should be done?

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Ignore my previous comment. I read it wrong.

In my previous experience, it seemed better to put unit tests in a separate assembly than putting them in the same assembly. There is some discussion about it here that highlight some of the same reasons I was thinking about. Having said that, there is an answer lower down that covers some ways to combine them together if the group prefers that approach. It will make the project more complex, but perhaps that is better than 2 projects. I'm open for discussion on this point for sure.

---

c-sharp/JsonUtils/MessageConverter.cs line 51 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

What is the purpose of these loops? It appears to just look at the base types of each object looking for a non-abstract base type, but in the possibilities list above you already ensure that the types in the list are not abstract. Is the problem that there are some non-abstract classes that are used as a base type?

The original lookup that generates possiblities goes arbitrarily deep in the class hierarchy. There will be immediate children and multiple levels down. For example, Message will have MessageEvent and MessageEventClientConnect generated as possibilities. I only want MessageEvent when I'm looking for "direct" message types. However, once I start looking at MessageEvent I'm going to run into MessageEventGeneric while walking up the inheritance hierarchy from MessageEventClientconnect, and that is an abstract type. The types returned all have to be constructed later, so I can't take in abstract classes.

The whole point of using reflection here is to avoid having to have people remember to add classes to the right dictionaries whenever a new message or event is added later. If people follow the README.md directions or just follow the existing pattern in the code, this will just work for them.

---

c-sharp/JsonUtils/MessageConverter.cs line 86 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Per our guidelines, var should only be used if it's obvious. Here it is not, so var should not be used.

Done - I thought it was obvious enough, but that's probably because I wrote it! 😁

---

c-sharp/JsonUtils/MessageConverter.cs line 93 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

nit: comments on their own line, see our guide. Here and all over the place.

Done - I was following the pattern already established in other parts of this file.

---

c-sharp/JsonUtils/MessageConverter.cs line 98 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I think it's dangerous to not keep the cloning of the state near where the reading happens (i.e. down where GetEventData is called). The cloning needs to happen before any call to ReadValue. It might be best to instead have the cloning happen inside of ReadValue so callers don't have to worry about it.

Done - that is a good suggestion!

---

c-sharp/JsonUtils/MessageConverter.cs line 102 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Can you still assert messageDataType is not null with the addition of L95-96? Above we throw if the TryGetValue fails but not here.

I don't see how messageDataType could be null here. We threw above if s_messageTypeMap didn't have a mapping to the right message type. If the mapping exists, then the value from the dictionary will not be null. The only way that variable could be null is if the dictionary didn't have any value, but we already threw if that was the case.

Am I missing something that you see?

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 22 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I'm not convinced that we should allow more than one handler for a specific event type. Only allowing one would greatly simplify things, I think.

I think forcing a single handler per event will force tighter coupling of other code later or at least will just push the multiplexing further out to be repeated. Events are one-way notifications as I understand them. If two different components need to be notified of something (e.g., a remote network object was created or destroyed, a user clicked on some button, etc.), then we need to provide some way for them both to learn about it later. If we start using events where we intended a request/response, that is a bug on the sender side, I would argue.

I am probably also thinking about events as I think about events in C#, Java, and other languages/frameworks where they are intended to be multicast, one way channels of information flow. If we don't intend that to be the definition here, then we probably need to start writing some architectural documentation that defines what we mean by key terms like "event" since it diverges from other commonly understood meanings of the same term.

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 70 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Without locking here, you can get into the situation where a Register or Unregister is half-run (i.e. between the TryGetValue and the setting of the new delegate). It won't necessarily cause problems, but it would still be best if a lock was consistently used (and it changed to be a normal dictionary). It could also keep potential problems happening in the future if more code is added to Register/Unregister.
The lock only needs to cover the TryGetValue call so it shouldn't cause any problems (although it will probably make the code a little awkward).

The proposed change doesn't do anything about the race condition you mention above because sometimes the Register/Unregister call will happen before the event is fired, and sometimes the Register/Unregister call will happen after the event is fired. The point of the lock is to avoid losing an event handler registration/unregistration since those are multi-step operations. Adding the lock to execution gives a false impression of doing something useful. If ordering between registration/unregistration and execution is needed, the owner of the PapiClient object has to enforce that.

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 63 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

If we decide to keep multiple message handlers (I'm hoping not), this should be changed to yield return calls instead of creating a new list all the time containing one item.

If we only need a single handler, would it make more sense to just change the interface from IEnumerable<Message>? to just Message?? That is how it was before this PR.

---

c-sharp/Messages/EventType.cs line 7 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

These 3 look like they are linking to TS items. If that's so can you add comments with where they came from?

That probably requires some input from @tjcouch-sil . Event types are scattered around in the TS code form what I could tell unlike message types. If we agree on where events should live in TS code I can certainly add comments/pointers here.

---

c-sharp/Messages/MessageEvent.cs line 16 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

usings should be used instead of using fully qualified names.

Normally I agree with you. For JSON in particular there is a nasty collision between Newtonsoft.Json and Text.Json. They share nearly identical types, and I lost 30-60 minutes in this code trying to debug a problem that was caused because VS decided I meant Newtonsoft Json here instead of System.Text.Json. I was trying to be SUPER explicit about which type I meant to avoid weird shenanigans.

---

c-sharp/Messages/MessageEventClientConnect.cs line 10 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

usings should be used instead of using fully qualified names.

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

---

c-sharp/Messages/MessageEventClientConnect.cs line 13 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

usings should be used instead of using fully qualified names.

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

---

c-sharp/Messages/MessageEventClientDisconnect.cs line 13 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

usings should be used instead of using fully qualified names.

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

---

c-sharp/Messages/MessageEventNetworkObject.cs line 12 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

usings should be used instead of using fully qualified names.

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

---

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

Previously, FoolRunning (Tim Steenwyk) wrote…

I'm fairly certain you can remove the _ = from this line.

The compiler generates warning CS4014 if you don't included the _ = to indicate you intend to throw away the value.

---

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

Previously, FoolRunning (Tim Steenwyk) wrote…

I think the answer is that we're fine, but since this is run asynchronously, we'll be sending data while we might be receiving data on another thread. Is that a problem?

The web socket implementation in .NET would have to be massively broken if this causes a problem. I'm used to doing this kind of thing with lower level networking sockets without any problems.

---

c-sharp-tests/c-sharp-tests.csproj line 1 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Thanks for adding the tests. We should add them to GHA in .github\workflows\test.yml so they run on each PR. Are you comfortable to do that? Either you or I could do that in a follow-up PR but it would be cool to see them on this PR.

I will work on this.

---

c-sharp-tests/c-sharp-tests.csproj line 15 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Any reason for preference of xunit over nunit?

I did some reading, and xunit seemed to be a slightly better choice from what I could tell as long as we didn't have any other reason to steer us one way or another. I don't feel strongly about it and am happy to change if we want to use nunit instead.

---

c-sharp-tests/Usings.cs line 1 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I did not realize you could do this. Fascinating.

I've never used XUnit. We probably need to discuss as a group the best C# testing framework to use (SIL usually uses NUnit).

Sure - Happy to discuss. As mentioned above I don't feel strongly about this. When I did some reading xunit seemed slightly better, but it isn't a massive difference.

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 7 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'd feel happier if the file was named the same as this class. A couple of times during review I've opened this file and been confused that it didn't match what I was seeing in Reviewable since this test file is named the same as the class file it's testing.

nit: I'd name this class in plural, i.e. MessageConverterTests.

Sure - In C++ I found it super helpful to keep unit tests that match the production code in identically named files in a parallel structure. That way it was clear where to look if we wanted to run code that was intended for specific production code. If we're careful with test class names (e.g., just append Tests at the end) it isn't really that different.

I'll make this adjustment.

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 49 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

It's part of Microsoft's best practices (I suggest we adopt the testing conventions as well).

Never seen those best practices before, but this all makes sense. I'll make the adjustments.

---

c-sharp-tests/MessageHandlers/MessageHandlerEvent.cs line 6 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'd feel happier if the file was named the same as this class.

Sure, same as earlier comments - I'll make the change.

[FoolRunning]

Reviewed 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @irahopkinson, @lyonsil, and @tjcouch-sil)

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, lyonsil wrote…

Do you want to create an issue in the backlog with the details of what you think should be done?

I've created an issue (#133)

---

c-sharp/JsonUtils/MessageConverter.cs line 51 at r1 (raw file):

Previously, lyonsil wrote…

The original lookup that generates possiblities goes arbitrarily deep in the class hierarchy. There will be immediate children and multiple levels down. For example, Message will have MessageEvent and MessageEventClientConnect generated as possibilities. I only want MessageEvent when I'm looking for "direct" message types. However, once I start looking at MessageEvent I'm going to run into MessageEventGeneric while walking up the inheritance hierarchy from MessageEventClientconnect, and that is an abstract type. The types returned all have to be constructed later, so I can't take in abstract classes.

The whole point of using reflection here is to avoid having to have people remember to add classes to the right dictionaries whenever a new message or event is added later. If people follow the README.md directions or just follow the existing pattern in the code, this will just work for them.

I see, now. It's because MessageEvent is still considered a Message object, so you need to make sure it derives from the correct type so it can be included in the correct Dictionary. That might deserve a comment somewhere in this method.
Or, maybe it should be done differently through interfaces? (i.e. have message instances and message events derive from completely different interfaces instead of relying on the base class?)
Any thoughts?

---

c-sharp/JsonUtils/MessageConverter.cs line 86 at r1 (raw file):

Previously, lyonsil wrote…

Done - I thought it was obvious enough, but that's probably because I wrote it! 😁

Yeah, according to the coding standards, basically you would only be able to use var if a direct assignment or a cast was on that line. Every other time, the type can't be inferred without knowing what a method returns. Personally, I try to never use var, but that's just me. 😉

---

c-sharp/JsonUtils/MessageConverter.cs line 118 at r3 (raw file):

    /// </summary>
    private static Enum<T> ReadValue<T>(Utf8JsonReader reader, string property)
        where T : class, EnumType

I originally wrote this method similar to what you have now, but I wasn't sure what happened if the method got inlined. I didn't find any answer to the question, and since the code depends on getting a copy of the reader, I didn't want to take any chances. Do you know if the reader is still guaranteed to be copied (via a struct copy from the caller) even if the ReadValue method gets inlined?

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 22 at r1 (raw file):

Previously, lyonsil wrote…

I think forcing a single handler per event will force tighter coupling of other code later or at least will just push the multiplexing further out to be repeated. Events are one-way notifications as I understand them. If two different components need to be notified of something (e.g., a remote network object was created or destroyed, a user clicked on some button, etc.), then we need to provide some way for them both to learn about it later. If we start using events where we intended a request/response, that is a bug on the sender side, I would argue.

I am probably also thinking about events as I think about events in C#, Java, and other languages/frameworks where they are intended to be multicast, one way channels of information flow. If we don't intend that to be the definition here, then we probably need to start writing some architectural documentation that defines what we mean by key terms like "event" since it diverges from other commonly understood meanings of the same term.

I agree that over the PAPI, we'd want more than one registration to be possible, but since the event registration is only C#-facing and the C# code is basically acting only as a data provider, I think it would be a bad design if we had more than one handler inside the C# that handled a given event from the PAPI.

It's possible I missed something, though.

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 70 at r1 (raw file):

Previously, lyonsil wrote…

The proposed change doesn't do anything about the race condition you mention above because sometimes the Register/Unregister call will happen before the event is fired, and sometimes the Register/Unregister call will happen after the event is fired. The point of the lock is to avoid losing an event handler registration/unregistration since those are multi-step operations. Adding the lock to execution gives a false impression of doing something useful. If ordering between registration/unregistration and execution is needed, the owner of the PapiClient object has to enforce that.

What I was trying to say, is that without the lock, some other thread could be running a register call and it could be inside the lock, but in between the two calls to the Dictionary and then this code here runs. Right now, the way the code currently is, I don't think that's a problem, but in the future, if something else was done in the lock to the Dictionary values (as an extreme example, maybe a new registration required all values in the Dictionary to be removed before the new ones were added back in), then this code here could get the Dictionary while in that in-between state since it's not inside a lock. Yes, the dictionary itself will be perfectly fine, but the code here will not run correctly because the information in the Dictionary was in the process of being changed.
Thus, I think it would be best if the lock was put around this call - which then makes it not need to be a concurrent dictionary.

If that's still not clear, feel free to talk on Discord.

---

c-sharp/Messages/MessageEventClientDisconnect.cs line 13 at r1 (raw file):

Previously, lyonsil wrote…

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

Ah, I missed that. Sorry

---

c-sharp/Messages/MessageEventNetworkObject.cs line 12 at r1 (raw file):

Previously, lyonsil wrote…

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

Ah, I missed that. Sorry.

---

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

Previously, lyonsil wrote…

The compiler generates warning CS4014 if you don't included the _ = to indicate you intend to throw away the value.

Ah. Makes sense.

---

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

Previously, lyonsil wrote…

The web socket implementation in .NET would have to be massively broken if this causes a problem. I'm used to doing this kind of thing with lower level networking sockets without any problems.

I haven't done much network programming so I wasn't sure. Good to have confirmation.

[lyonsil]

Reviewable status: 27 of 29 files reviewed, 16 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 7 at r1 (raw file):

Previously, lyonsil wrote…

Sure - In C++ I found it super helpful to keep unit tests that match the production code in identically named files in a parallel structure. That way it was clear where to look if we wanted to run code that was intended for specific production code. If we're careful with test class names (e.g., just append Tests at the end) it isn't really that different.

I'll make this adjustment.

Done

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 49 at r1 (raw file):

Previously, lyonsil wrote…

Never seen those best practices before, but this all makes sense. I'll make the adjustments.

Done

---

c-sharp-tests/MessageHandlers/MessageHandlerEvent.cs line 6 at r1 (raw file):

Previously, lyonsil wrote…

Sure, same as earlier comments - I'll make the change.

Done

[lyonsil]

Reviewable status: 27 of 29 files reviewed, 16 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

c-sharp/JsonUtils/MessageConverter.cs line 51 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I see, now. It's because MessageEvent is still considered a Message object, so you need to make sure it derives from the correct type so it can be included in the correct Dictionary. That might deserve a comment somewhere in this method.
Or, maybe it should be done differently through interfaces? (i.e. have message instances and message events derive from completely different interfaces instead of relying on the base class?)
Any thoughts?

Because of the tie-ins with JSON deserialization and because C# doesn't allow inheriting from multiple classes, I'm not sure of a clearly better approach. If we go with purely interfaces and not inheritance that is going to introduce code duplication between the message types. Also technically these events are messages in that all message share the same low-level properties. So events ought to implement IMessage, and we'd be back in the same boat.

I haven't thought of a better way to do what I've got here. I don't think this is bad per se, and if you follow the pattern you don't have to dig into it and understand the details. But yes, there is some complexity here. I will try to add better comments unless we come up with a different approach altogether.

[lyonsil]

Reviewable status: 27 of 29 files reviewed, 16 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

c-sharp/JsonUtils/MessageConverter.cs line 118 at r3 (raw file):
From the C# language reference:

When you pass a structure-type variable to a method as an argument or return a structure-type value from a method, the whole instance of a structure type is copied.

There is nothing in the language reference about inlining breaking this behavior. Other searches in StackOverflow suggest that inlining is not possible when struct arguments are part of the signature. The discussions are several years old, but still, if the language spec says they get copied I would trust that without second guessing.

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 22 at r1 (raw file):

I think it would be a bad design if we had more than one handler inside the C# that handled a given event from the PAPI.

To me, it seems too early to make that declaration. You've thought about this more than me, but I've found it's better to not make too many assumptions early in a project about how all the code will work exactly. The normal use case very well might be a single subscriber. The high level idea of a one way information stream doesn't naturally say "this should only go to 1 receiver," though, even just within C#.

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 70 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

What I was trying to say, is that without the lock, some other thread could be running a register call and it could be inside the lock, but in between the two calls to the Dictionary and then this code here runs. Right now, the way the code currently is, I don't think that's a problem, but in the future, if something else was done in the lock to the Dictionary values (as an extreme example, maybe a new registration required all values in the Dictionary to be removed before the new ones were added back in), then this code here could get the Dictionary while in that in-between state since it's not inside a lock. Yes, the dictionary itself will be perfectly fine, but the code here will not run correctly because the information in the Dictionary was in the process of being changed.
Thus, I think it would be best if the lock was put around this call - which then makes it not need to be a concurrent dictionary.

If that's still not clear, feel free to talk on Discord.

I think I understand what you're saying. I do think the code works fine as-is, as you say, but it does add potential fragility for future maintainers. We probably don't need the perf benefits of avoiding an uncontested lock which I expect to be the normal case, so it isn't that big of a deal.

[lyonsil]

Reviewable status: 23 of 29 files reviewed, 16 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 70 at r1 (raw file):

Previously, lyonsil wrote…

I think I understand what you're saying. I do think the code works fine as-is, as you say, but it does add potential fragility for future maintainers. We probably don't need the perf benefits of avoiding an uncontested lock which I expect to be the normal case, so it isn't that big of a deal.

Done

---

c-sharp-tests/c-sharp-tests.csproj line 15 at r1 (raw file):

Previously, lyonsil wrote…

I did some reading, and xunit seemed to be a slightly better choice from what I could tell as long as we didn't have any other reason to steer us one way or another. I don't feel strongly about it and am happy to change if we want to use nunit instead.

Switched to nunit to be more in line with other SIL projects. The degree to which xunit is arguably better is probably negated and more by not being standard.

[irahopkinson]

Looking good, just need valid json and I'm happy.

Reviewed 1 of 1 files at r2, 2 of 2 files at r3, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @FoolRunning and @lyonsil)

---

.vscode/extensions.json line 9 at r1 (raw file):

Previously, lyonsil wrote…

Done - yes, good idea.

Great. It's now missing an ending comma. Saving in VS Code would auto format it to remove the last dangling comma as well to make it valid JSON.

---

.vscode/settings.json line 2 at r1 (raw file):

Previously, lyonsil wrote…

Isn't it already in alphabetical order here?

haha, yes, sorry, 'd' does come before 'e'.

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, lyonsil wrote…

In my previous experience, it seemed better to put unit tests in a separate assembly than putting them in the same assembly. There is some discussion about it here that highlight some of the same reasons I was thinking about. Having said that, there is an answer lower down that covers some ways to combine them together if the group prefers that approach. It will make the project more complex, but perhaps that is better than 2 projects. I'm open for discussion on this point for sure.

We definitely don't want to complicate the setup. I was just trying to avoid having another folder in our repo root. Moving the existing c-sharp to c-sharp/src and c-sharp-tests to c-sharp/tests would do that but my aversion to too much in the root might just be me. Unless others think so too we can leave it as is for this PR.

---

c-sharp/JsonUtils/MessageConverter.cs line 102 at r1 (raw file):

Previously, lyonsil wrote…

I don't see how messageDataType could be null here. We threw above if s_messageTypeMap didn't have a mapping to the right message type. If the mapping exists, then the value from the dictionary will not be null. The only way that variable could be null is if the dictionary didn't have any value, but we already threw if that was the case.

Am I missing something that you see?

Doesn't out Type? on L92 indicate that eventMessageDataType could be undefined? Oh wait, but then the TryGetValue would return false and we wouldn't assign it. Sorry, my mistake.

---

c-sharp/Messages/EventType.cs line 7 at r1 (raw file):

Previously, lyonsil wrote…

That probably requires some input from @tjcouch-sil . Event types are scattered around in the TS code form what I could tell unlike message types. If we agree on where events should live in TS code I can certainly add comments/pointers here.

Gotcha, out of scope for this PR.

---

c-sharp-tests/JsonUtils/MessageConverter.cs line 49 at r1 (raw file):

Previously, lyonsil wrote…

Done

Nice job on the change and thanks for the link. I've added it to our guide.

[lyonsil]

Reviewable status: 28 of 30 files reviewed, 10 unresolved discussions (waiting on @FoolRunning and @irahopkinson)

---

.vscode/extensions.json line 9 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Great. It's now missing an ending comma. Saving in VS Code would auto format it to remove the last dangling comma as well to make it valid JSON.

oops - fixed now

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

We definitely don't want to complicate the setup. I was just trying to avoid having another folder in our repo root. Moving the existing c-sharp to c-sharp/src and c-sharp-tests to c-sharp/tests would do that but my aversion to too much in the root might just be me. Unless others think so too we can leave it as is for this PR.

I think the idea of c-sharp/src and c-sharp/tests being separate is a good one. It's not a big deal, but it is a little cleaner.

This might be a nice follow up PR as it will have so many moves and small text adjustments just for the renaming.

---

c-sharp-tests/c-sharp-tests.csproj line 1 at r1 (raw file):

Previously, lyonsil wrote…

I will work on this.

Done

[lyonsil]

Reviewable status: 21 of 30 files reviewed, 10 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @lyonsil)

---

c-sharp/JsonUtils/MessageConverter.cs line 51 at r1 (raw file):

Previously, lyonsil wrote…

Because of the tie-ins with JSON deserialization and because C# doesn't allow inheriting from multiple classes, I'm not sure of a clearly better approach. If we go with purely interfaces and not inheritance that is going to introduce code duplication between the message types. Also technically these events are messages in that all message share the same low-level properties. So events ought to implement IMessage, and we'd be back in the same boat.

I haven't thought of a better way to do what I've got here. I don't think this is bad per se, and if you follow the pattern you don't have to dig into it and understand the details. But yes, there is some complexity here. I will try to add better comments unless we come up with a different approach altogether.

After some more thought I still couldn't come up with a better idea, so I added some detailed comments in this file to help future readers. If you have a better idea after some thought let me know.

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 63 at r1 (raw file):

Previously, lyonsil wrote…

If we only need a single handler, would it make more sense to just change the interface from IEnumerable<Message>? to just Message?? That is how it was before this PR.

I changed the IMessageHandler interface to drop nullability and changed all the implementing classes to use yield as you suggest. I also changed the consuming classes to remove the null checks. It did simplify the code in most places which was nice.

[lyonsil]

Reviewable status: 20 of 30 files reviewed, 9 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @lyonsil)

---

c-sharp/AssemblyInfo.cs line 3 at r1 (raw file):

Previously, lyonsil wrote…

I think the idea of c-sharp/src and c-sharp/tests being separate is a good one. It's not a big deal, but it is a little cleaner.

This might be a nice follow up PR as it will have so many moves and small text adjustments just for the renaming.

Created #138 to track this as a separate issue.

[lyonsil]

@FoolRunning I dug more into the .NET websocket implementation based on your previous comments/questions. There isn't a problem sending and receiving at the same time as I said ought to be true for reasonable network socket implementations. However, there is a problem with 2+ threads sending at the same time which is disappointing. (It would also be a problem to have 2+ threads receiving at the same time, but our code doesn't do that.) To avoid expanding this PR beyond the intention of event handling, I opened #137 as a follow up to deal with the problem of multiple senders. We shouldn't have problems with this right away, but we should get back to it sooner rather than later.

Reviewable status: 20 of 30 files reviewed, 9 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @lyonsil)

[irahopkinson]

I really like the removal of nullable. Although I wouldn't have know to suggest that, I do like the results in the simpler code.

Yeh, for C# GHA tests. 👏

Reviewed 2 of 2 files at r6, 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @FoolRunning and @lyonsil)

---

.github/workflows/test.yml line 54 at r7 (raw file):

        run: npm run build:data-release

      - name: run dotnet unit tests

nit: I would put these after the format check since they will get bigger and bigger but the check won't likely take much more time - that way we would fail earlier.

So good to see these tests running in GHA, thanks.

[FoolRunning]

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

---

c-sharp/JsonUtils/MessageConverter.cs line 51 at r1 (raw file):

Previously, lyonsil wrote…

After some more thought I still couldn't come up with a better idea, so I added some detailed comments in this file to help future readers. If you have a better idea after some thought let me know.

I was meaning having interfaces in addition to the existing base classes where the interfaces would only be on the outer-most classes in the hierarchy. Either way, I think it's fine for now. We can always change it later since it's just an implementation detail of the C#.

---

c-sharp/JsonUtils/MessageConverter.cs line 118 at r3 (raw file):

Previously, lyonsil wrote…

From the C# language reference:

When you pass a structure-type variable to a method as an argument or return a structure-type value from a method, the whole instance of a structure type is copied.

There is nothing in the language reference about inlining breaking this behavior. Other searches in StackOverflow suggest that inlining is not possible when struct arguments are part of the signature. The discussions are several years old, but still, if the language spec says they get copied I would trust that without second guessing.

Good enough for me

---

c-sharp/MessageHandlers/MessageHandlerEvent.cs line 22 at r1 (raw file):

Previously, lyonsil wrote…

I think it would be a bad design if we had more than one handler inside the C# that handled a given event from the PAPI.

To me, it seems too early to make that declaration. You've thought about this more than me, but I've found it's better to not make too many assumptions early in a project about how all the code will work exactly. The normal use case very well might be a single subscriber. The high level idea of a one way information stream doesn't naturally say "this should only go to 1 receiver," though, even just within C#.

Yeah, that is a good point. Let's just leave it for now. I'm not thrilled with the complications it introduces to the code, but it's fine for now.

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 63 at r1 (raw file):

Previously, lyonsil wrote…

I changed the IMessageHandler interface to drop nullability and changed all the implementing classes to use yield as you suggest. I also changed the consuming classes to remove the null checks. It did simplify the code in most places which was nice.

I like these changes. Thanks for doing that.

---

c-sharp/Messages/MessageEvent.cs line 16 at r1 (raw file):

Previously, lyonsil wrote…

Normally I agree with you. For JSON in particular there is a nasty collision between Newtonsoft.Json and Text.Json. They share nearly identical types, and I lost 30-60 minutes in this code trying to debug a problem that was caused because VS decided I meant Newtonsoft Json here instead of System.Text.Json. I was trying to be SUPER explicit about which type I meant to avoid weird shenanigans.

Ah. I didn't think about that.

---

c-sharp/Messages/MessageEventClientConnect.cs line 10 at r1 (raw file):

Previously, lyonsil wrote…

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

Ah. I didn't think about that.

---

c-sharp/Messages/MessageEventClientConnect.cs line 13 at r1 (raw file):

Previously, lyonsil wrote…

The code fails to compile with error CS0120 if you don't prepend this with Messages. It thinks I'm talking about the property exposed in MessageEvent.cs named EventType instead of the class Messages.EventType. I didn't provide the full typename of Paranext.DataProvider.Messages.EventType because the first two parts weren't needed, only the Messages portion to distinguish between the two.

Ah. I didn't think about that.