Remove the default output buffer limit #45
Conversation
| @@ -240,10 +240,9 @@ extension OutputProtocol where Self == FileDescriptorOutput { | |||
| @available(SubprocessSpan, *) | |||
| #endif | |||
| extension OutputProtocol where Self == StringOutput<UTF8> { | |||
| /// Create a `Subprocess` output that collects output as | |||
| /// UTF8 String with 128kb limit. | |||
| /// Create a `Subprocess` output that collects output as UTF8 String | |||
There was a problem hiding this comment.
I think the documentation comment here should say that the buffer is unlimited, so the amount of memory required is proportional to the size of the output.
…limited buffer size
|
Updated header doc to indicate that |
| @@ -46,7 +46,7 @@ public protocol OutputProtocol: Sendable, ~Copyable { | |||
| #endif | |||
| extension OutputProtocol { | |||
| /// The max amount of data to collect for this output. | |||
| public var maxSize: Int { 128 * 1024 } | |||
| public var maxSize: Int { .max } | |||
There was a problem hiding this comment.
@iCharlesHu Why are we fixing a hang with increasing the buffer size to infinity? That's not quite the right fix I think.
if you're not streaming and you blow through the max size you should get an error, not a hang. That's also what AsyncProcess does.
in fact, you will now see a hang (followed by an OOM kill) if you run for example cat /dev/zero.
There was a problem hiding this comment.
I understand your point, but struggling a bit to find the right answer here. If we put a limit in, and during testing someone only uses file sizes < 128k (for example), then a customer of the app is the one who hits the error. If we are unlimited, then it'll just use infinite memory, which is also a problem.
There was a problem hiding this comment.
Safe and unexpected is better than unsafe an unexpected IMHO. So a limit is IMHO a must.
There was a problem hiding this comment.
If you get an error telling you "too much data to collect", it's immediately actionable. If your process just grinds to a halt or gets OOM killed, you need to do much more work to even figure out what the problem is.
|
We decided to keep the default limit but throw an error when the buffer limit is reached (instead of simply stop reading, which could lead to child process being hang) |
This PR removes the output buffer limit by default when setting
.stringor.bytesasoutputorerror.Resolves #44