Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libsubprocess: Support line buffering stdout/stderr #2262

Merged
merged 8 commits into from Jul 24, 2019

Conversation

@chu11
Copy link
Contributor

commented Jul 23, 2019

To limit callbacks to the on_stdout and on_stderr callbacks, support the ability to specify if stdout/stderr/channel output should be line buffered.

Originally, I made this option to "enable" line buffering. But upon further investigation, everything in flux reads lines. So I enabled line buffering by default, but allow it to be disabled.

Lots of stupid cleanups/fixes along the way.

@chu11 chu11 force-pushed the chu11:issue2224 branch 2 times, most recently from a2aaf60 to c023460 Jul 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

re-pushed, fixed something a little racy in one of the tests

@garlick

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Excellent! I'll have a peek and a poke.

Copy link
Member

left a comment

Couple minor suggestions.

You can prob merge the two commits containing inline doc fixes.

I find myself a bit lost without more detailed commit messages.

So what is the net effect on the I/O code in the shell? Fewer on_stdin, on_stdout callbacks when a read_line will just return 0 (and it's not eof yet)? On master I'm unable to prove to myself that this is happening right now. It was happening a lot before as I recall. Hmm, maybe I'm not checking the right thing.

src/common/libsubprocess/util.c Outdated Show resolved Hide resolved
src/common/libsubprocess/util.c Outdated Show resolved Hide resolved
chu11 added 8 commits Jul 23, 2019
In a number of tests, arguments were passed to ok() for output but
no conversion specifies were specified in the output format.  Remove
this unnecessary arguments.
Add additional tests to test for the length of the data coming
back from flux_subprocess_read_line().
Update or reformat comments for clarity.  Also fix
English errors as needed.
Do not want an ENOMEM error logging to stderr, ENOMEM should simply
be returned to the caller.
Refactor checks to determine if user set a BUFSIZE parameter.  It
can be called fewer times by moving the call to a common function.
By default, line buffer stdout/stderr before calling output callbacks.

Support LINE_BUFFER cmd option, so that line buffering can be enabled/disabled
as desired.

Fixes #2224
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

So what is the net effect on the I/O code in the shell? Fewer on_stdin, on_stdout callbacks when a read_line will just return 0 (and it's not eof yet)? On master I'm unable to prove to myself that this is happening right now. It was happening a lot before as I recall. Hmm, maybe I'm not checking the right thing.

Yup, you're exactly correct. We saw it in #2211.

One of the new tests I added shows it pretty well. I have something that outputs "hi" 2200 times and then outputs a newline (I picked 2200 b/c I wanted > 4096 bytes to pop out).

With line buffering turned on, on_stdout is called 2 times, once for the line, once for EOF. With line buffering turned off, on_stdout is called some random number of times around 170-180 times.

@chu11 chu11 force-pushed the chu11:issue2224 branch from c023460 to dcea5af Jul 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

just re-pushed, fixed up some commit messages, squashed the comment updates together into one commit, removed to errno = ENOMEM per comments above.

@codecov-io

This comment has been minimized.

Copy link

commented Jul 23, 2019

Codecov Report

Merging #2262 into master will increase coverage by <.01%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #2262      +/-   ##
==========================================
+ Coverage   80.73%   80.74%   +<.01%     
==========================================
  Files         210      210              
  Lines       33224    33233       +9     
==========================================
+ Hits        26825    26835      +10     
+ Misses       6399     6398       -1
Impacted Files Coverage Δ
src/common/libsubprocess/util.c 100% <100%> (+7.69%) ⬆️
src/common/libsubprocess/local.c 79.28% <100%> (+2.66%) ⬆️
src/modules/job-exec/bulk-exec.c 74.51% <100%> (ø) ⬆️
src/common/libsubprocess/remote.c 69.27% <33.33%> (-0.73%) ⬇️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/common/libutil/veb.c 96% <0%> (-2.86%) ⬇️
src/broker/module.c 74.94% <0%> (-0.24%) ⬇️
src/common/libflux/message.c 80.88% <0%> (ø) ⬆️
src/common/libsubprocess/server.c 71.42% <0%> (+0.56%) ⬆️
Copy link
Member

left a comment

LGTM!

@garlick

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

When you're ready feel free to set the merge-when-passing label.

@mergify mergify bot merged commit ba2c232 into flux-framework:master Jul 24, 2019
4 checks passed
4 checks passed
Summary 1 rule matches
Details
codecov/patch 90.47% of diff hit (target 80.73%)
Details
codecov/project 80.74% (+<.01%) compared to 690fe2b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.