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

common/libflux: Add buffer readonly setting #1548

Merged
merged 1 commit into from Jun 15, 2018

Conversation

Projects
None yet
3 participants
@chu11
Copy link
Contributor

chu11 commented Jun 15, 2018

As discussed in #1546 and #1547, this adds a "drain" flag that indicates users are allowed to finish reading everything in the buffer, but can no longer write to it. It should be useful in #1547 to indicate to users that a buffer is closed and no longer available for writing.

Note that I decided to use the word "drain" for this setting. It's the best word I could come up with given the usage. Terms like "close" or "complete" didn't seem quite right. Open to suggestions of a different word.

Also, the errno I chose to return when a user attempts to write to the buffer when drain is set, is EROFS, i.e. read only file system. Not the most perfect errno, but I couldn't think of a better one.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 15, 2018

Coverage Status

Coverage increased (+0.03%) to 79.382% when pulling c2eea95 on chu11:issue1546-buffernowriteflag into 8196c53 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 15, 2018

I can see why you struggled to come up with a good term for a read-only buffer, and this is why I gave up on adding "close" to flux_buffer_t interface (the idea of it just doesn't fit). However, drain might not be the best choice for the public API because this term is already used in other similar software, e.g.

  • Python 3 async IO Streams
  • Node.js streams drain event, which is emitted when a filled buffered stream is ready for writing again
  • Rust's std:Vec:drain method which removes specified items from a vector (drains them)

Maybe the drain flag could be renamed to simply "readonly"? That is literally what the "drain" flag actually is, and fits better mnemonically with EROFS error returned from a read-only buffer.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 15, 2018

Otherwise, seems like a nice simple approach. I also like how avoiding the term "close" let's us ignore what happens when flux_buffer_read_to_fd is called on a read-only buffer.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 15, 2018

Restarted a hung builder

common/libflux: Add buffer readonly setting
Add a readonly setting that allow users to read all remaining data
in the buffer, but not allow any additional data to be written into
the buffer.  When users attempt to write data into the buffer when
readonly has been set, they get an errno of EROFS.

@chu11 chu11 force-pushed the chu11:issue1546-buffernowriteflag branch from 136a055 to c2eea95 Jun 15, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jun 15, 2018

I like the idea of calling it "readonly", simple and obvious. I went ahead and made the change, squashed changes since it's a pretty simple/obvious change (minor comment changes adjusting for English).

One minor logic change is the "is_readonly" function now just returns if the buffer is readonly or not and doesn't check if the buffer is empty. The original "is_drained" function meant something somewhat different English wise.

@chu11 chu11 changed the title common/libflux: Add buffer drain setting common/libflux: Add buffer readonly setting Jun 15, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 15, 2018

Restarted a builder that hit #1077

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 15, 2018

Great! This will be useful in #1547. Once Travis reports back I'll merge.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jun 15, 2018

hmmm, one builder hung, but did have a failure with t0001-basic.t 55 - instance can stop cleanly with subscribers (#1025). That's a new one.

expecting success: 
	flux start ${ARGS} -s2 --bootstrap=selfpmi bash -c "nohup flux event sub hb &"

lt-flux-broker: module 'kvs' was not cleanly shutdown
lt-flux-broker: module 'barrier' was not cleanly shutdown
lt-flux-broker: module 'job' was not cleanly shutdown
lt-flux-broker: module 'resource-hwloc' was not cleanly shutdown
lt-flux-broker: module 'aggregator' was not cleanly shutdown
flux-event: flux_event_subscribe: Success
flux-start: 0 (pid 19176) Killed
not ok 55 - instance can stop cleanly with subscribers (#1025)

I doubt it's due to anything I did in this PR. Something racy? restarting the builder

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jun 15, 2018

oh, we raced trying to restart the builder :-) I guess I flubbed my search, didn't see #1077.

@grondo grondo merged commit 569fe2d into flux-framework:master Jun 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 79.382%
Details
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 15, 2018

Ok, merged! Thanks!

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.