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

libflux: add flux_buffer_write_watcher_close() #1547

Merged
merged 4 commits into from Jun 16, 2018

Conversation

Projects
None yet
5 participants
@grondo
Copy link
Contributor

grondo commented Jun 14, 2018

As discussed in #1546, add a simple method for scheduling a call to close(2) once buffer has been emptied for flux buffer write watchers. This is basically the simplest approach for now while we continue to discuss alternatives.

Also added is the reactorcat test case, which implements something like cat(1) with the Flux reactor and buffer watchers.

Still need to add unit tests and a sharness test driver for reactorcat.

Fixes #1546

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jun 14, 2018

Just scanned through this and LGTM.

@grondo grondo force-pushed the grondo:buffer-watcher-close branch from 32a92bc to b4abde3 Jun 14, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #1547 into master will increase coverage by 0.08%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
+ Coverage   79.08%   79.17%   +0.08%     
==========================================
  Files         169      169              
  Lines       31004    31035      +31     
==========================================
+ Hits        24520    24571      +51     
+ Misses       6484     6464      -20
Impacted Files Coverage Δ
src/common/libflux/reactor.c 93.15% <100%> (+1.3%) ⬆️
src/common/libflux/ev_buffer_write.c 89.58% <92.85%> (+0.1%) ⬆️
src/common/libflux/keepalive.c 86.66% <0%> (-6.67%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/bindings/lua/flux-lua.c 82.15% <0%> (-0.7%) ⬇️
src/common/libflux/request.c 88.46% <0%> (ø) ⬆️
src/modules/kvs/kvs.c 66.03% <0%> (+0.16%) ⬆️
src/broker/module.c 84.07% <0%> (+0.27%) ⬆️
src/common/libflux/msg_handler.c 87.72% <0%> (+0.36%) ⬆️
src/common/libflux/future.c 89.42% <0%> (+0.44%) ⬆️
... and 5 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 14, 2018

Coverage Status

Coverage increased (+0.09%) to 79.463% when pulling 630927d on grondo:buffer-watcher-close into 569fe2d on flux-framework:master.

{
if (w) {
struct ev_buffer_write *evw = w->data;
evw->eof = true;

This comment has been minimized.

@chu11

chu11 Jun 14, 2018

Contributor

Should we add if (!evw->eof && !evw->closed) so users can't close multiple times?

This comment has been minimized.

@grondo

grondo Jun 14, 2018

Author Contributor

I thought it should be idempotent. However, you are right might as well check for evw->closed otherwise eof is reset and the close callback could be called again.

Should the function therefore have a return value and return EINVAL if evw->closed and EINPROGRESS if evw->eof && !evw->closed?

This comment has been minimized.

@chu11

chu11 Jun 14, 2018

Contributor

Yeah, that's what I was thinking, that the close callback could be called multiple times. This function returns void, so should the errno matter?

@@ -40,7 +42,16 @@ static void buffer_write_cb (struct ev_loop *loop, ev_io *iow, int revents)
if (flux_buffer_read_to_fd (ebw->fb, ebw->fd, -1) < 0)
return;

if (!flux_buffer_bytes (ebw->fb))
if (!flux_buffer_bytes (ebw->fb) && ebw->eof) {
if (close (ebw->eof) < 0)

This comment has been minimized.

@chu11

chu11 Jun 14, 2018

Contributor

should be close (ebw->fd)?

This comment has been minimized.

@grondo

grondo Jun 14, 2018

Author Contributor

Yipes, what a ridiculously ridiculous typo. Funny thing is in my reactorcat test this actually worked because it resolved to close (1) or close (STDOUT_FILENO). (Luckily would be caught by the unit tests, but nice catch @chu11!)

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Jun 14, 2018

Chatted with @grondo in the hall, after this PR I will add a "I'm finished with this buffer" flag into flux_buffer_t to handle the issue of If data is written to the underlying flux_buffer_t after this call, the results are undefined.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 14, 2018

@chu11, if you want to add the flux_buffer_close() call we could merge that PR first and then it could be utilized in this PR. (or we could do it as one PR)

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Jun 14, 2018

I was just thinking it would be a separate PR, but lemme see if I can whip something up quick.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 14, 2018

I was just thinking it would be a separate PR, but lemme see if I can whip something up quick.

Up to you but it would be nice to have the feature so it could be used in this PR. If you submit a PR I'll merge it and then use it in this one.

I still have to write tests here (and make the fixes you caught), so you have some time ;-)

@grondo grondo force-pushed the grondo:buffer-watcher-close branch from b4abde3 to 862de31 Jun 14, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 14, 2018

@chu11, pushed a commit to address your comments. flux_buffer_write_watcher_close() now returns and integer and indicates failure for these cases:

  • Invalid watcher handle: EINVAL
  • eof already set: EINPROGRESS
  • closed: EINVAL

Though it likely won't be useful to differentiate, it seemed simple to do and will aid in testing.

Also fixed the typo in the call the close 🤦‍♂️

Now to write some tests.

@grondo grondo force-pushed the grondo:buffer-watcher-close branch from a35c27e to 822011d Jun 15, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 15, 2018

Just added a small change to mark buffer read-only when flux_buffer_write_watcher_close() is called, and a couple tests to ensure this new code after close.

I also added 1 small test to t0001-basic.t to ensure the reactor-based cat test works to shuttle data from input to output and that EOF before any data (e.g. reading from /dev/null) also works as expected.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 15, 2018

If this PR is ready, I'll want to first squash the incremental changes before merge.

@chu11
Copy link
Contributor

chu11 left a comment

overall looks good to me, just a few nits I noticed.

flux_watcher_t *flux_buffer_write_watcher_create (flux_reactor_t *r, int fd,
int size, flux_watcher_f cb,
int flags, void *arg);

flux_buffer_t *flux_buffer_write_watcher_get_buffer (flux_watcher_t *w);

/* "write" EOF to buffer write watcher 'w'. The underlying fd will be closed
* once the buffer is emptied. If data is written to the underlying
* flux_buffer_t after this call, the results are undefined.

This comment has been minimized.

@chu11

chu11 Jun 15, 2018

Contributor

"If data is written to the underlying flux_buffer_t after this call, the results are undefined." can probably be removed now. Or if we want to, we can say attempts to write to the buffer result in EROFS.

This comment has been minimized.

@grondo

grondo Jun 15, 2018

Author Contributor

Good catch, thanks!

ok (flux_buffer_write (fb, "shouldfail", 10) == -1 && errno == EROFS,
"buffer: flux_buffer_write after close fails with EROFS");

flux_watcher_start (w);

This comment has been minimized.

@chu11

chu11 Jun 15, 2018

Contributor

should probably add a flux_buffer_write_watcher_is_closed() == 0 test case before the reactor run?

This comment has been minimized.

@grondo

grondo Jun 15, 2018

Author Contributor

Good suggestion, will do.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 15, 2018

Ok, @chu11, added a couple quick commits to address your comments.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Jun 15, 2018

everything LGTM, lets squash, then merge it

@grondo grondo force-pushed the grondo:buffer-watcher-close branch from 6c35a63 to eb12d46 Jun 15, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 15, 2018

Thanks @chu11, squashed and force-pushed.

grondo added some commits Jun 14, 2018

reactor: add flux_buffer_write_watcher_close
Add a close method to the flux_buffer_write_watcher interface. The
close method writes a pending EOF to the buffer watcher, which will
automatically close the underlying file descriptor once the buffer
is empty.

After the fd is closed, the user's callback is called with FLUX_POLLOUT
and a new accessor flux_buffer_write_watcher_is_closed () can be called
to retrieve the errno (if any) from the close(2) system call.

Fixes #1547
test: add reactorcat buffered IO test case
Add a new test command reactorcat to t/reactor/reactorcat which uses
Flux buffered IO watchers to implement at cat(1) tool.
test: libflux/reactor: cover flux_buffer_write_watcher_close
Add coverage to test_reactor.t for flux_buffer_write_watcher_close()
and flux_buffer_write_watcher_is_closed().
testsuite: add sanity check for reactorcat test code
Ensure that the sample use case in t/reactor/reactorcat.c works
for piping data from one file to another, and also that the reactor-
based cat works without data (eof only).

@grondo grondo force-pushed the grondo:buffer-watcher-close branch from eb12d46 to 630927d Jun 16, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 16, 2018

Not sure what was going on, but Travis didn't initiate tests on the latest version of this PR. I've just force pushed again to wake it up.

@chu11 chu11 merged commit 1a3ff37 into flux-framework:master Jun 16, 2018

4 checks passed

codecov/patch 97.14% of diff hit (target 79.08%)
Details
codecov/project 79.17% (+0.08%) compared to 569fe2d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 79.463%
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jun 16, 2018

Thanks!

@grondo grondo deleted the grondo:buffer-watcher-close branch Jul 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.