DataTransport protocol improvements

[jscheid]

Hi, the current DataTransport protocol works but could be improved in some ways. (You might already have planned to do so as part of your async work?)

1. The setReaderHandler function means that the constructor doesn't have access to the reader, which means the class variable holding the reader has to be declared optional, which means the code gets complicated by checks to see if it has been set.
2. The write function isn't declared throws, meaning that it's forced to report errors through a side channel. It's also not compatible with async except with fire-and-forget or reporting back through a side channel.
3. Errors during read also have to be reported through a side channel, meaning that ProtocolTransport or other objects taking a DataTransport as an input either have to be made aware of the side channel somehow, or will be oblivious to errors.

One way of addressing the above might be to make the protocol look more like this:

public protocol DataTransport {
    typealias ReadHandler = (Data) async throws -> Void

    func write(_ data: Data) async throws -> Void
    func readLoop(_ onData: @escaping ReadHandler) async throws -> Never
}

write should be self-explanatory. readLoop would keep reading until the Task is cancelled, which would replace the close function in the current protocol version. And the reader, too, can throw if there's a problem. (I think it best for the function to never return and throw on EOF, to make EOF handling more explicit and so that there is only one way for the loop to exit, but this is more or less a matter of taste.)

The ReadHandler in the proposal is also changed to allow throw and async. I think this would let us eventually remove the dispatch queue we talked about on #2 and enable adding backpressure. Implementing that change could be left for another time, but I figured it best to change the protocol already in the interest of keeping breaking changes to a minimum.

All of this obviously isn't compatible with Swift <5.5, I'm not sure how big of a problem that is to you. Nothing speaking against dropping compatibility from my end, but you might have other plans.

Curious to hear what you think about all this.

[mattmassicotte]

I am also unhappy with this implementation, but have not given it a lot of thought. I have absolutely no problem with bumping Swift versions.

Now that I'm looking at it, I'm not even sure a protocol is the right approach at all. This is really just two functions, and we can probably get way with capturing that as a struct. If you do not need to protocol-ness (as a generic constraint for example), is much more flexible.

I'm also inclined to look at AsyncSequence. It is clearly the direction Apple is going. Though, it is pretty problematic when crossing API boundaries due to the lack of any support or a type-erased option. So, I'm not sure this is the ideal way to do it.

public struct DataTransport {
    public let write: (Data) async throws -> Void
    public let data: AsyncSequenceOfSomeKind { get throws }
}

What do you think of this?

[jscheid]

Oh right, yeah that's even better. In fact I have an AsyncSequence wrapper here for ProtocolTransport as well.

Not sure I follow what you mean about lack of any support. Wouldn't it just be a sequence with Element == Data?

[mattmassicotte]

The issue is there is no AnyAsyncSequence and you cannot use any AsyncSequence<Element> (because AsyncSequence does not declare a primary associated type).

This can be a problem at API boundaries. You are forced to expose concrete AsyncSequence types in your APIs. Sometimes it isn't a big deal, but I've found it to be extremely inconvenient, and also can require a lot of boilerplate to fix. Apple has at least acknowledged the issue. For now, there are a number of libraries created to try to address the issue.

https://github.com/vsanthanam/AnyAsyncSequence https://github.com/sideeffect-io/AsyncExtensions https://github.com/reddavis/Asynchrone

[jscheid]

Ah, right, that makes sense. Thanks for filling in that background.

What do you think of using an AsyncStream<Data> for now? It wouldn't be quite as flexible or elegant but should let people do everything they need, as far as I can tell, even if they might have to build a wrapper (which apparently can be done with AnyPublisher if I follow the conversation.

Personally I'd stay away from the libraries you've mentioned as a dependency for this library. If there was one library that everybody uses then perhaps, but with multiple to choose from it feels pretty messy.

[mattmassicotte]

I'm into using AsyncStream<Data> as long as it's behind a typealias. The odds of breakage isn't terribly high, but that even further helps to insulate clients. Hopefully this gets fixed with the next major swift releases anyways.

I just just finished removing https://github.com/Flight-School/AnyCodable as a dependency and I'd like to avoid adding new ones if possible.

[jscheid]

Awesome, it's a plan! Now, I think this all would amount to touching a lot of different areas across the core, can I leave that to you or is there anything I can help with to make this happen?

[mattmassicotte]

ha the million dollar question!

Right now, I'm in the middle of a refactor multi-library refactor that started here. This was the motivation for the removal of AnyCodable and the modification of the handler properties of ProtocolTransport. I think I can look into doing this after that's done. But, if you want to take this on, that would be completely fine with me!

[jscheid]

OK, thank you! Then let's just let it sit here for a while, it's no big rush from my end. I might take a stab if I can find the time but can't promise anything.

[mattmassicotte]

Sounds good to me!

[mattmassicotte]

Alright, so this ended up becoming a problem for some other work in a dependent library, so I took a stab at this. Not fully worked out yet, but would love to know what you think of this new JSONRPCSession type.

[jscheid]

Hi Matt, thanks for following up. I'm afraid that I've since abandoned the project that prompted me to look into all this. I'm still happy to provide feedback on your changes, as best as I can without having it fresh in mind. I don't see a link or PR, would you mind pointing me at it?

[mattmassicotte]

Ah, well I hope you got to abandon it for something more fun!

Looking is totally up to you, I've not released anything yet, but have put my work onto main. If you want, you can have a look here:

https://github.com/ChimeHQ/JSONRPC/blob/main/Sources/JSONRPC/JSONRPCSession.swift

Absolutely no pressure.

[jscheid]

Ah, no problem. I've abandoned it because it was a project to scratch an itch, and it so happens someone else scratched it before I could, a welcome result.

Still, I'll try finding time to take a good look at your code. At first glance it looks great, I love that you've stuck with the plan to build it around sequences.

I've noticed that in the file you linked to, in three places errors are handled by a print statement. I'd suggest trying to find a way to bubble up these error conditions so they can be handled by the library consumer.

The errors might indicate a misconfiguration or communication problem that isn't caught by lower transport layers. As a library consumer I'd probably want to close the connection and reconnect, or throw the error into the user's face, rather than risk incoming messages to be silently ignored, or outgoing messages to be silently dropped. (Silent in the sense that for a GUI app, the errors only appear in the debug console where they wouldn't be visible to end users.)

[mattmassicotte]

Great suggestion on the error visibility. I've added an AsyncSequence that publishes these, so they can be monitored if needed. I think at this point I'm going to call this issue complete!

Thanks again for the suggestion and for looking!