Handle large stdout and stderr lines from scripts #70
Conversation
This file contains 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
Currently if a script outputs a single line to stdout or stderr over 16k characters the script is stopped with a pipe error. When used in combination with shell scripts this makes for a very confusing error state where output is just limited or the process either hangs or fails without explanation. This change replaces the shell executer based on github.com/go-cmd/cmd with one using the standard libraries os/exec package. Further more the line buffer limit is increased to 512k characters. go-cmd does not support changing the maximum line length so the library cannot support our use case. This bug was found when a Go service was testing an HTML template. The test would output the rendered HTML template and a complex SVG exceeded the current 16k limit. This resulted in the test being marked as successfull by the Go toolchain but as the stdout pipe was broken shell scripts would not work as intended.
|
I think this might break up the order of script's stdout and stderr. Hold on... |
|
A couple of issues with this implementation. Using I'll have to give it some more work before its ready. Closing until then. |
Closed
Crevil
added a commit
that referenced
this pull request
Mar 29, 2022
This change partially reverts 48c5180 (#70) which fixed an issue where large outputs from shell scripts would break output pipes (#90). The change replaced go-cmd with a native implementation which on the surface fixed the large output bug but introduced another subtle race condition in the output of scripts. The motivation in the original commit for replacing go-cmd was the missing ability to configure the output buffers size. That feature is now available in go-cmd why this change set reverts to use the proven go-cmd implementation. This removes a lot of complexity from shuttle while also fixing the race condition identified in #90.
Crevil
added a commit
that referenced
this pull request
Mar 29, 2022
This change partially reverts 48c5180 (#70) which fixed an issue where large outputs from shell scripts would break output pipes (#90). The change replaced go-cmd with a native implementation which on the surface fixed the large output bug but introduced another subtle race condition in the output of scripts. The motivation in the original commit for replacing go-cmd was the missing ability to configure the output buffers size. That feature is now available in go-cmd why this change set reverts to use the proven go-cmd implementation. This removes a lot of complexity from shuttle while also fixing the race condition identified in #90.
Crevil
added a commit
that referenced
this pull request
Mar 30, 2022
This change partially reverts 48c5180 (#70) which fixed an issue where large outputs from shell scripts would break output pipes (#90). The change replaced go-cmd with a native implementation which on the surface fixed the large output bug but introduced another subtle race condition in the output of scripts. The motivation in the original commit for replacing go-cmd was the missing ability to configure the output buffers size. That feature is now available in go-cmd why this change set reverts to use the proven go-cmd implementation. This removes a lot of complexity from shuttle while also fixing the race condition identified in #90.
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.
Currently if a script outputs a single line to stdout or stderr over 16k
characters the script is stopped with a pipe error. When used in combination
with shell scripts this makes for a very confusing error state where output is
just limited or the process either hangs or fails without explanation.
This change replaces the shell executer based on github.com/go-cmd/cmd with one
using the standard libraries os/exec package. Further more the line buffer limit
is increased to 512k characters.
go-cmd does not support changing the maximum line length so the library cannot
support our use case.
This bug was found when a Go service was testing an HTML template. The test
would output the rendered HTML template and a complex SVG exceeded the current
16k limit. This resulted in the test being marked as successfull by the Go
toolchain but as the stdout pipe was broken shell scripts would not work as
intended.