Remove the default collected output buffer limit and throw an error when the limit is reached #130
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
parkera
reviewed
Jul 29, 2025
| @@ -70,18 +71,20 @@ extension SubprocessError { | |||
| return 6 | |||
| case .asyncIOFailed(_): | |||
| return 7 | |||
| case .failedToSendSignal(_): | |||
| case .outputBufferLimitExceeded(_): | |||
There was a problem hiding this comment.
After we set the API we should probably consider the error numbers to be frozen, in case people have written documentation about them or compare them to raw values.
There was a problem hiding this comment.
I agree. I'm only doing this because we haven't tagged 0.0.1 yet.
parkera
reviewed
Jul 29, 2025
parkera
reviewed
Jul 29, 2025
| if maxLength != .max { | ||
| // If we actually have a max length, attempt to read one | ||
| // more byte to determine whether output exceeds the limit | ||
| maxLength += 1 |
There was a problem hiding this comment.
Does this mean that we've drained 1 more byte from the I/O than the caller expected?
There was a problem hiding this comment.
Yes. Unfortunately this is the only way to detect whether there's more data coming. If the caller was only expected N bytes, then the child process should only process N bytes with EOF at the end. If not then we considered this an error.
parkera
reviewed
Jul 29, 2025
…hen the limit is reached
iCharlesHu
force-pushed
the
charles/buffer-limit-error
branch
from
2577a37 to
ba88b42
Compare
jakepetroules
approved these changes
Jul 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #44 , remove the default output limit since we can't really specify one. Also throw an error if output exits limit.
Resolves: #44