Fix ringio example package + o.Range.Length

[joeycumines]

What does this change do?

This change addresses issues identified with the ringio.Bounded implementation,
mostly but not only relating to behavior when the overwrite option is used.

Bug fixes:

1. o.Range.Length now returns zero if start is greater than end
2. Fix panic in ringio.Bounded.Bytes (and String) caused by unsafe slicing
3. Fix panic and incorrect behavior in ringio.Bounded.Write (overwrite case)

Behavior changes:

1. ringio.Bounded.Read now returns io.EOF on empty
2. ringio.Bounded.Bytes (and String) no longer consumes any data (still copies)

Other changes:

1. Fixed typo in o.Ring.PushN doc (it may return ErrEmpty, not ErrFull)

Tests for the change

See the unit tests added to ringio.

Checklist for code changes

• Is this a code change? If so:
• Made a test for the change
• Check project test coverage remains at 100%
• Added an entry to CHANGELOG.md

[antifuchs]

Great catches and tests, thanks! I do have a quibble with the semantics of 0, nil vs. 0, io.EOF though & would mildly prefer the original behavior.

[antifuchs]

Good catch! But we should follow go convention & handle the exceptional case in the if block:

Suggested change

	if r.End > r.Start {
	return r.End - r.Start
	}
	return 0
	if r.End < r.Start {
	return 0
	}
	return r.End - r.Start

[joeycumines]

Flatter code is a general programming convention. I'm curious to know what makes you describe it as a Go convention, could you provide a link?

That said, it's your repo, and it's not like it makes any difference to me. I'll invert it.

[antifuchs]

Oooh, yep, verrrry good catch. However, the ring buffer being empty at the point when the read happens doesn't mean it's terminally empty. From the docs to io.Reader:

Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0. Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.

Discouraged, yep, but I believe these are the semantics we're dealing with: Nothing happened but the ring might be readable in the future.

[antifuchs]

To that end,

Suggested change

	return 0, io.EOF
	return 0, nil

[antifuchs]

I think it would be reasonable to add a Close method to Ring, so that any future reads can return 0, io.EOF though!

[joeycumines]

@antifuchs I changed this because your existing implementation results in a broken io.Reader implementation, which is to say it will work very poorly with any implementation that reads in a loop, and uses an error as the terminating case. Specifically, it will result in a tight loop doing nothing.

Examples:

• io.ReadAll
• io.Copy (and equivalents)
• bytes.Buffer.ReadFrom (also used by io.Copy and similar)

Presumably, this is one reason why it is discouraged. The io.EOF error is simply a sentinel value, indicating that there is no more data to be read. Your current implementation is a fixed buffer, and is logically never opened or closed, much the same way as bytes.Buffer is never opened or closed.

Supporting blocking behavior would be best implemented by the caller, IMO, e.g. using io.Pipe, or net.Pipe. Both those implementations use the same style implementation (using a ping-pong pattern, via channels), in order to mitigate deadlocks, which IIRC persisted in io.Pipe for a very long time. Implementing that functionality within the ring buffer (instead of just letting the caller implement it, fairly trivially) would significantly impact it's fit for (what I would consider) this implementation's core use cases. My own use case, included.

[joeycumines]

As an example of an implementation that doesn't follow a strict "returns io.EOF on close" pattern, consider tar.Reader.Read, which returns io.EOF to bound each archived file, of which they may be more than one, within the input stream.

[antifuchs]

Hm, yeah you make a good point there. I feel that the go stdlib could use some clarification about what the "you're at the end" behavior of an io.Read should be - but if we can just re-use an existing Read object (which it seems to pointedly avoid discussing), the behavior you introduced makes sense - and if it fixed io.ReadAll, even better!

[joeycumines]

This one's waiting on you @antifuchs :)