Flush stdout buffer when writing data

[gsabran]

Flush stdout buffer when writing data. I noticed that without an extra line break, the receiver was not getting the message written to stdout.

As a nit (happy to revert) I thought that stdio was clearer that stdioPipe as afaik piping is specific to shell scripts, and stdin can be written to in other contexts.

[mattmassicotte]

I'm afraid this isn't a safe change. Introducing new data like this will break some existing communication protocols. However! I do think the underlying problem you discovered should still be fixed. Perhaps we can do it another way?

(and yes I 100% agree with your nit, thanks for calling that out!)

[gsabran]

Introducing new data like this will break some existing communication protocols.

This only applies to when using the process' stdio though. ie other communication protocols (for ex websocket) are not impacted.

As an alternative, what do you think of

@available(*, deprecated, renamed: "stdio", message: "Use stdio instead")
public static func stdioPipe() -> DataChannel {
  stdio(flushOnWrite: false)
}

public static func stdio(flushOnWrite: Bool = true) -> DataChannel { ... }

[mattmassicotte]

This only applies to when using the process' stdio though. ie other communication protocols (for ex websocket) are not impacted.

That's true! This transport just happens to be really important for my own uses.

I think it is critical that transports faithfully write only the data that a user actually intends. I'm much more inclined to use an out-of-band mechanism here like fflush. This would have the exact same behavior but not alter the data stream in any way.

What do you think about keeping non-flushing behavior the default, so existing clients continue to work, and look into using fflush?

[gsabran]

I'm not too familiar with fflush, but from what I understood that direction makes sense. How would you incorporate it here?

[mattmassicotte]

Ok, I did little more research, and I think the correct API is probably fsync. This flushes file descriptors, and I believe it is possible to get the file descriptor from a FileHandle.

However, I also believe it is possible that FileHandle may already have an API to do this!

https://developer.apple.com/documentation/foundation/filehandle/3172531-synchronize

[gsabran]

Interesting. I tried adding try FileHandle.standardOutput.synchronize() and this fails with Error Domain=NSCocoaErrorDomain Code=512 UserInfo={NSUnderlyingError=0x600003604db0 {Error Domain=NSPOSIXErrorDomain Code=45}}

[gsabran]

So as you initially suggested, what do you think of instead using fflush(stdout) ?

[mattmassicotte]

Huh intersting! So, I believe that POSIX error 45 translates to ENOTSUP. Maybe you cannot use synchronize on these. Does fflush work?

[gsabran]

it seems that fflush works yes

[gsabran]

Unrelated to this specific PR, as I've been working on JRPC over stdio, I implemented two changes that I wanted to mention in case you'd be interested to have them upstreamed:

• when launching a process that will be connected to over stdio, make it easier to launch the process with the same environment as in the interactive shell. Otherwise when used within a MacOS app, most likely you won't have the right PATH.
• when reading from stdio, address the issue where long JRPC messages might be received over several data chunks as stdout is flushing in between. The issue was that each data chunk was sent to JSONRPC while some were not valid JSON (incomplete)

[mattmassicotte]

it seems that fflush works yes

Ok that's great! Are you up for incorporating it into this PR? It's probably fine to enable this by default, but I think keeping it optional and off by default is the most compatible change.

So if you are up for reworking this to use fflush, and have it be optional and disabled by default, I think should

• when launching a process that will be connected to over stdio, make it easier to launch the process with the same environment as in the interactive shell. Otherwise when used within a MacOS app, most likely you won't have the right PATH.

Yes, I am well-aware of this problem. I have a solution here that works quite well across a number for different shells: https://github.com/ChimeHQ/ProcessEnv. This stuff is definitely a pain, but I think this is pretty independent of JSON-RPC.

• when reading from stdio, address the issue where long JRPC messages might be received over several data chunks as stdout is flushing in between. The issue was that each data chunk was sent to JSONRPC while some were not valid JSON (incomplete)

Hmm this is an interesting problem! In my use of JSON-RPC, the actual payloads are wrapped in a lower-level protocol that guarantees you know how much data to read before the message is fully-delivered (very similar to HTTP). But I think some kind of DataChannel wrapper or other solution could definitely be useful and I'm very open to it!

The implementation I came up with involves a small wrapper around a custom AsyncSequence, so it looks at least superficially-similar to what you are doing here.

https://github.com/ChimeHQ/LanguageServerProtocol/blob/main/Sources/LanguageServerProtocol/Framing/DataChannel%2BMessageFraming.swift

[gsabran]

Given that this is a new function, I enabled flushing by default. This seems to be the most reasonable and the less likely to cause unexpected issues.

[mattmassicotte]

Sorry for taking so long here, and I agree that makes sense!