Ordering between incoming notifications and requests

[koliyo]

I'm working with a higher level RPC protocol that has some semantic dependency between specific notifications and requests.

So it is important that if I can start processing the notification before the request, if they arrive from the client in that order.

In the JSONRPCSession.startMonitoringChannel the incoming sequence channel.dataSequence is ordered, but the dispatcher splits this into the requestSequence and notificationSequence.

So any user of JSONRPCSession can not have any guarantee of the ordering of notifications and requests. (To clarify, this is the ordering between the two sets, the internal ordering of requests and notifications are of course ordered.)

I understand the reasoning of splitting up the two sequences, and it makes the API pretty clean with nice separation. But do you see any way of handling notification-request ordering requirements for me as an JSONRPCSession user?

I can not change the semantics of the protocol itself, ie removing the strict ordering requirement in the high level protocol.

[mattmassicotte]

First, thank you so much for taking the time to write this up. I really appreciate it. I actually ran into a very similar problem while testing a downstream component (https://github.com/ChimeHQ/LanguageServerProtocol/blob/c5309f528bbe604cbd37f8ef7b928a45410b066c/Sources/LanguageServerProtocol/Additions/MockServer.swift#L11).

And now that you bring this up, I'm interested in fixing this. I this the session probably should preserve all ordering in one single stream. I think it should be up to the client to separate these out and handle them independently if needed. Unifying these into one single stream with a enum-based payload should be possible, I think a change here will be disruptive, but this AsyncSequence-based API is pretty new anyways.

However, I do have some other tasks I have to attend to first. I will try to have a look at this next week.

[mattmassicotte]

My first thought here was to refactor the request, notification, and error streams all into one single sequence. The type would be an enum that lets you distinguish between these three events. Does that sound like it could work for you @koliyo ?

[koliyo]

Yup, that definitely seems like a reasonable approach!

Maybe also think about error sequence. I'm not sure I need ordering for that compared to notifications and requests, but it might be required at some point?

[mattmassicotte]

Ok great! Yes, errors would be included and ordering would be preserved. I figured that's easy to ignore if unneeded.

In terms of scheduling I have a few things I have to take care of first, but I may be able to take a look at this next week.

[mattmassicotte]

Ok I took a shot at getting this done. It wasn't too bad. Also found an area that was unintentionally introducing some async behavior that is unneeded.

On the "feature/single-sequence" branch: https://github.com/ChimeHQ/JSONRPC/tree/feature/single-sequence

[koliyo]

I have looked a bit at the single sequence branch, and I think the refactoring looks very good!

It is pretty breaking change though, so I guess the major version number should be bumped as part of this (?), although we are still pre-1.0.

In general it would be very helpful if this branch was merged and then a new tagged release was made for JSONRPC with the other DataChannel PRs as well that are now in main. Having a proper release makes it much easier for downstream usage 😅

[koliyo]

Question: I see you removed the weak self reference in the changes. Do we still need the weak reference in the downstream LanguageServerProtocol usage after this? Not sure I fully understand this mechanism.

I have a branch in my LanguageServerProtocol fork without the weak self:

    private func startMonitoringSession() async {
        let seq = await session.eventSequence

        for await event in seq {
                        //...

Or is this problematic, do we still need the old pattern with weak self?

    private func startMonitoringSession() async {
        let noteSequence = await session.notificationSequence

        self.notificationTask = Task { [weak self] in
            for await (notification, data) in noteSequence {
                guard let self = self else { break }

See https://github.com/koliyo/LanguageServerProtocol/blob/main/Sources/LanguageServerProtocol-Server/Impl.JSONRPCServer.swift#L29

[mattmassicotte]

In general it would be very helpful if this branch was merged and then a new tagged release was made for JSONRPC with the other DataChannel PRs as well that are now in main. Having a proper release makes it much easier for downstream usage 😅

That's funny, I was actually delaying this specifically so I didn't interfere with your downstream work. If you think this looks good, I'm ok to move forward.

You are right this is a pretty large breaking change, however there are tags for older versions if someone really needs them, and I think most users (if there are any!) would be ok with this direction.

[mattmassicotte]

Just checking in again - I'm happy to do this, I just want confirmation that you've had a chance to try it out!

I'd like to help out with the migration here in downstream libraries too!

[koliyo]

Yup, I definitely think this is better, with a common ordered sequence, and I am already using this new API. So feel free to tag a new version!

[mattmassicotte]

Ok, version 0.9.0 is now available! I'm really happy this is done!

I modified its Package.swift to make sure it does not pull in this change. I'm now very excited to get https://github.com/ChimeHQ/LanguageServerProtocol all fixed up. However, in an effort to minimize the amount of conflicts you have to deal with, I'm trying to hold off as long as I can.

[koliyo]

Hm, so the 0.9.0 release says Unified event stream, but the single-sequence branch was never merged back to main and is not part of this? 🧐

[mattmassicotte]

Goddammit I messed it up. Took the lazy-but-slightly-dangerous approach of deleting and recreating the tag.