ChimeHQ / JSONRPC

Swift library for JSON-RPC
BSD 3-Clause "New" or "Revised" License
28 stars 8 forks source link

Don't deliver responses out of band #2

Closed jscheid closed 1 year ago

jscheid commented 1 year ago

Great library, thanks! I'd like to suggest this improvement:

This change ensures that responses and notifications are dispatched in the order they are received with respect to each other. This is important especially for pubsub, where the response to the initial subscription request carries the subscription ID, knowledge of which is required to dispatch later subscription updates to the correct recipient.

mattmassicotte commented 1 year ago

I'm so happy to hear that it's useful!

At first, I didn't understand this change, but I think I do get it now. The problem was relayResponse was enqueuing the work, so not only is this second enqueue unnecessary, it was also allowing these bits of work to interleave?

jscheid commented 1 year ago

Yes, that's right, or rather responses vs notifications will remain interleaved but won't be dispatched out of order anymore (when they arrive in quick succession.)

I'm actually wondering why there's a dispatch queue at all, considering the transport reader would already be running on an async thread. Having backpressure is usually a good thing in a network reader. Anyway, probably best to do one thing at a time.

mattmassicotte commented 1 year ago

Less about need for parallelism, more about protecting the responders and id properties. But, a queue isn't the best synchronization mechanism. I think this could be improved quite a bit actually - lots of stuff in here that predates async/await. In fact, I just adding support for it recently.

This is great, thank you so much! And, I'd be happy to continue talking about further improvements!

jscheid commented 1 year ago

Thanks for the quick merge!

I think it's working really well actually. A few minor improvements I can think of (e.g. for exclusive use as a client, a request shouldn't need to be decodable, a response shouldn't need to be encodable.)

Beyond that I do have a layer on top of it that I was thinking of contributing when it's a bit more polished. It adds the following:

  1. Slightly more ergonomic API based around these protocols:
protocol InvocationRequest: Codable {
    static var methodName: String { get }
    associatedtype Response: Codable
}

protocol SubscriptionRequest: Codable {
    static var methodName: String { get }
    associatedtype Update: Decodable
}
  1. Type-safe subscriptions, which uses a new Decoder implementation that can decode a JSONValue (from an AnyJSONRPCNotification) into a Decodable.

Aiming to open a PR for discussion at some point.

mattmassicotte commented 1 year ago

Well I'm very glad to hear it! I have a lot of miles on it, but basically all are part of of LSP library (https://github.com/chimeHQ/LanguageServerProtocol), which probably isn't the most typical usage.

I see you've been working on some of these already and I'm pumped!