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: rename/cleanup read_eof_reached() add flux_subprocess_read_until_eof() #2265

Merged
merged 11 commits into from Jul 25, 2019

Conversation

@chu11
Copy link
Contributor

commented Jul 24, 2019

This PR does two things (it could be split into two PRs, but keeping it as one given #2255 thought process).

After a lot of thought on how to 1) make it easier to deal with EOF scenarios with read streams and 2) how to make it easier / more convenient for callers, in this PR:

  • The flux_subprocess_read_eof_reached() temporary function that was introduced in PR #2254 is now "promoted" into a real library function because there are a few circumstances it is useful and the convenience function (described below) couldn't' be used. Renamed it to flux_subprocess_read_stream_closed(), added header usage information, added tests.

  • Added a new convenience function flux_subprocess_read_line_until_eof(), that will always read a line from the stream, but once EOF has been reached, it can return a non-line for the last read. Then finally, it'll return length = 0 when EOF has been reached. The reason this could not be done with flux_subprocess_read_line() is because flux_subprocess_read_line() still needs to work on streams that are both line and non-line buffered. This convenience function will fail with errno = EPERM (good errno?) if you try to read from a non-line buffered stream. Updated usage in 4 locations (broker, flux-exec, shell, job-exec).

  • Had to fix one line buffering corner case bug I found along the way.

  • Some stupid fixes along the way as always.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

The overall approach seems good, however I wonder if we could shorten some of the function names.

What if we renamed flux_subprocess_read_line_until_eof to flux_subprocess_read_line() and made the new flux_subprocess_read_line() into flux_subprocess_getline(). The main reason being simply that it would be nice if the more commonly used function was a bit shorter (I think the getline style usage would not be very common)

Just curious, what does flux_subprocess_read_stream_closed return if there are still bytes to be read, but EOF is pending?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

What if we renamed flux_subprocess_read_line_until_eof to flux_subprocess_read_line() and made the new flux_subprocess_read_line() into flux_subprocess_getline(). The main reason being simply that it would be nice if the more commonly used function was a bit shorter (I think the getline style usage would not be very common)

I wonder if re-naming should be tied into the function signature discussion in #2253, since its also "read_line" and "read_trimmed_line" in the buffer lib. Perhaps flux_subprocess_read_line_until_eof() could be renamed to flux_subprocess_getline()?

Just curious, what does flux_subprocess_read_stream_closed return if there are still bytes to be read, but EOF is pending?

Good question, it returns > 0 when EOF/closed has been noticed. Number of bytes left to be read doesn't matter. I should probably add something to the header comments about that.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

re-pushed, adding a bit more comments on flux_subprocess_read_stream_closed() and renamed read_line_until_eof() to getline() if that's ultimately the function name people like :-) Will squash later.

@garlick

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

This looks OK to me, to the extent I understand what's going on. I may be offline a bit today and tomorrow and the weekend, so if this gets in a mergeable state, don't let me hold anything up.

@grondo
grondo approved these changes Jul 25, 2019
Copy link
Contributor

left a comment

Nice work! I didn't have any immediate comments and the getline() call did clean up code in several places. Thanks!

@grondo

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Feel free to squash away if you still need to do so, and add the merge-when-passing label.

@chu11 chu11 force-pushed the chu11:issue2255 branch from a51b680 to d2e8a9a Jul 25, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

thanks, repushed and set merge-when-passing

@codecov-io

This comment has been minimized.

Copy link

commented Jul 25, 2019

Codecov Report

Merging #2265 into master will decrease coverage by 0.03%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
- Coverage   80.81%   80.78%   -0.04%     
==========================================
  Files         213      210       -3     
  Lines       33412    33239     -173     
==========================================
- Hits        27002    26851     -151     
+ Misses       6410     6388      -22
Impacted Files Coverage Δ
src/shell/task.c 89.28% <100%> (-1.25%) ⬇️
src/common/libsubprocess/local.c 79.43% <100%> (+0.14%) ⬆️
src/common/libsubprocess/subprocess.c 88.38% <100%> (+0.96%) ⬆️
src/modules/job-exec/bulk-exec.c 75% <50%> (+0.48%) ⬆️
src/broker/runlevel.c 81.39% <50%> (+0.71%) ⬆️
src/cmd/flux-exec.c 75.79% <50%> (+0.01%) ⬆️
src/common/libsubprocess/remote.c 69.6% <83.33%> (+0.32%) ⬆️
src/modules/job-manager/job-manager.c 64% <0%> (-2.04%) ⬇️
src/common/libflux/mrpc.c 87.79% <0%> (-1.19%) ⬇️
... and 15 more
chu11 added 11 commits Jul 23, 2019
Update flux_subprocess_read_line() with additional information on newline
existing in buffer.
Add missing EINVAL tests for flux_subprocess_read_trimmed_line().
Rename flux_subprocess_read_eof_reached() to
flux_subprocess_read_stream_closed().  Add additional header information
on function usage.
Add tests for flux_subprocess_read_stream_closed().
Add coverage by using flux_subprocess_read_stream_closed() in rexec
tests.
Update flux_standard_output() to use new
flux_subprocess_read_stream_closed() function.
Add a flag to struct subprocess_channel, indicating if the read buffer
in the struct is line buffered.
Add new convenience function flux_subprocess_getline() that
handles common libsubprocess usage.

Update usage in cmd/flux-exec, modules/job-exec, broker, and shell to use
new convenience function.

Add comments to few locations indicating when it shouldn't be used.

Fixes #2255
Fix a line buffering corner case with remote subprocesses.  If the
last line of remote output is not terminated by a newline, the
output callback (e.g. on_stdout) can be called multiple times until
EOF is received.
@SteVwonder SteVwonder force-pushed the chu11:issue2255 branch from d2e8a9a to b095403 Jul 25, 2019
@grondo

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Restarted travis builder that hit valgrind test issue #2195

@mergify mergify bot merged commit 94cf968 into flux-framework:master Jul 25, 2019
3 checks passed
3 checks passed
Rule: rebase and merge when passing all checks (merge) The pull request has been merged automatically
Details
Summary 1 rule matches
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
4 participants
You can’t perform that action at this time.