Reusable DataChannel extensions

[koliyo]

The DataChannel abstraction allows many different implementations. Question is how to publish and allow these implementations to be reused by others. I have currently implemented three new data channels:

• https://github.com/koliyo/JSONRPC-DataChannel-UniSocket
  • Mainly for local unix socket (--pipe argument when using VS Code extension)
  • Allow logging to stdout while keeping protocol transport on a socket
• https://github.com/koliyo/JSONRPC-DataChannel-StdioPipe
  • Typical stdio pipe mode
• https://github.com/koliyo/JSONRPC-DataChannel-Actor
  • IPC communication using Swift actors
  • Useful when testing LSP backend with LSP client code, both in Swift and running in the same process

These are currently implemented as standalone repositories.

To me, it seems reasonable to bundle a few basic datachannel implementations as part of the JSONRPC repo itself. You already have DataChannel+LocalProcess.swift and DataChannel+UserScript.swift. And then you also have DataChannel+HostedProcess.swift which is overlapping with these.

I think my contributions with DataChannel-StdioPipe and DataChannel-Actor could potentially be added directly to JSONRPC, since they are very simple mechanisms and does not have any external dependencies.

The DataChannel-UniSocket is more specific, with external library dependency, and should probably be a separate library.

Thoughts on this?

[mattmassicotte]

I think the first thing we really need to discuss is managing transitive dependencies. I think about this a lot, because the Swift community is extremely dependency-averse in general. I've even written a bit on the topic, and I think it will be relevant. A library that pulls in others is much less likely to be used.

I find this very frustrating! But it is critical from a practical perspective. Ok, so with that out of the way. I think we should just internalize most of the DataChannel bits. It just make sense.

UniSocket

I wasn't able to examine the dependency because it appears point to a package not references by the repo itself. But the code seems really small. So, I think it's totally fine to keep this particular one external.

StdioPipe

I think this one should definitely be moved into JSONRPC.

Actor

I honestly think we should include this one too. It's really cool! However, this one pulls in apple-collections. I believe we can use a very simple protocol to break the dependency here. And, in this case, I believe it is worth the complexity, as this particular channel is highly-specialized and unlikely to see widespread use.

I spent a tiny out of time looking at your use of Deque. I think it could be replaced with a generic constrained to the following protocol:

protocol ReceiveQueue : Collection {
    init(minimumCapacity: Int)
    func append(_ element: Element)
}

UserScript, LocalProcess

Both of my custom channels cannot currently be included because they depend on framing from a higher-level piece. But, your framing abstraction can fix that, I believe. And I just like it a lot more anyways.

So, let's do this. Can you create a PR that adds StdioPipe?

If you are up for doing another for Actor, that'd be awesome. But, because I'm asking for a lot more work, I volunteer to do it for you (though I'd need your help with testing). Just let me know which option you'd like.

[koliyo]

Ok, sounds reasonable wrt dependencies!

I have created a PR for stdio datachannel.

I also made an attempt at implementing the protocol based Actor without explicit Deque usage. I am afraid my limited swift experience is not enough, regarding the associatedtypes/generic handling.

I have made an attempt here, but it does not yet work:

https://github.com/koliyo/JSONRPC/tree/feature/actor-datachannel

In addition, I tried adding swift-collections as a test-only dependency, but that does not seem to exist in swift?

https://stackoverflow.com/questions/41401753/test-only-dependencies-when-using-the-swift-package-manager

Anyway, please take a look if you can make it work, or if you would consider swift-collections to be an ok dependency. Otherwise the package could be a separate package.

Regarding testing, I have a small test for actor datachannel:

https://github.com/koliyo/JSONRPC-DataChannel-Actor/blob/main/Tests/JSONRPC-DataChannel-ActorTests/JSONRPC_DataChannel_ActorTests.swift

And this is also the reason for the numBlocked member in the DataActor, which otherwise would not be needed.

[mattmassicotte]

Correct - there is no way to have test-only dependencies. Very annoying. Here's my attempt. I made two significant changes.

First, I just removed the init(minimumCapacity: Int) requirement, and instead opted to defer queue creation entirely to the client via a closure.

Second, I made the DataActor type non-public. This seemed ok to me given the API. But, that's a significant change, though is easy to fix.

https://github.com/ChimeHQ/JSONRPC/tree/feature/dataactor

[koliyo]

Ok, non-public DataActor is completely fine imo. I think your variant is fine to use, but also a little more burden on the user with the additional queueProvider setup. But I am fine with that, and it is also more flexible, like what you demonstrate with using Array instead of Deque.

Actually what we can do is use your generic implementation, but provide a default implementation using Array, in which case we do not need any additional package dependency.

I have done some modifications to your branch, and created a PR with the added default factory method, so now the user only needs to do DataChannel.withDataActor() to use the Array implementation.

[mattmassicotte]

Great changes! I know the closure-based queueProvider was annoying, but it was required for compatibility with non-Sendable queue types. I use this trick quite a lot to get around the inability to pass non-Sendable values into an actor initializer.