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

proposal: testing/iotest: add ErrWriter #54111

Open
Fryuni opened this issue Jul 28, 2022 · 10 comments
Open

proposal: testing/iotest: add ErrWriter #54111

Fryuni opened this issue Jul 28, 2022 · 10 comments

Comments

@Fryuni
Copy link

Fryuni commented Jul 28, 2022

Add an iotest.ErrWriter io.Writer implementation that accepts up to n bytes and then fails with the provided error.

This would be useful for quickly testing implementations that receive an io.Writer and deals with some of the possible errors.

This proposal is similar to #38781, but for io.Writer instead of io.Reader. But the API here would take the specified number of bytes to discard before failing.

There seems to be multiple cases where a writer is implemented just for this, as seen here (even more than the reader, which was surprising to me)

package iotest

// ErrWriter returns an io.Writer that accepts the first n bytes and then fails with err for all Write calls.
func ErrWriter(n int, err error) io.Writer

Rationale behind the API

The API difference from ErrReader is that the reader don't receive the bytes it should return, but that can be achieved with this composition:

io.MultiReader(
	bytes.NewReader([]byte("content")),
	iotest.ErrReader(io.ErrClosedPipe),
)

There is no implementation on the standard library (that I could find) that allows writing n bytes to a writer and then the remaining to the next writer. io.MultiWriter writes everything to all the given writers.

iotest.TruncateWriter would be my guess on what to use to make these tests, but it fails silently after n bytes.

Another API I considered was ErrWriter(writer io.Writer, n int, err error) io.Writer to allow testing what was written until the error, but when needed that can be achieved with:

buf := new(bytes.Buffer)
io.MultiWriter(
	buf,
	iotest.ErrWriter(1024, io.ErrClosedPipe),
)

Of course, the opposite would also work by using io.Discard when the data is not wanted, I'd be ok with either.

@gopherbot gopherbot added this to the Proposal milestone Jul 28, 2022
@ianlancetaylor

This comment was marked as resolved.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 28, 2022
@Fryuni

This comment was marked as resolved.

@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

This sounds a little like io.LimitWriter.
Most of our APIs use Readers, but any that return Writers might be helped by having a composable limit.

@seankhliao
Copy link
Member

There was #46465 for a LimitedWriter

@Fryuni
Copy link
Author

Fryuni commented Aug 4, 2022

That seems to me like a specific use case for limiting the writing in the actual code, returning an io.EOF after a set amount of bytes.

This proposal is for testing, which allows defining any error to be returned.

@earthboundkid
Copy link
Contributor

earthboundkid commented Aug 19, 2022

Moving this over from #54548, which was closed as a dupe:

package io

func LimitWriter(w Writer, limit int64, err error) Writer {
	return &limitedWriter{w, limit, err}
}

type limitedWriter struct {
	w Writer
	n int64
	e error
}

func (lw *limitedWriter) Write(p []byte) (int, error) {
	if lw.n < 1 {
		return 0, lw.e
	}
	if lw.n < int64(len(p)) {
		p = p[:lw.n]
	}
	n, err := lw.w.Write(p)
	lw.n -= int64(n)
	return n, err
}

Cf. #51115


I think there are cases where one ends up using a limit reader, but conceptually what you really want is a limit writer. You don't really care if the request you're copying is consumed some kind of streaming processor, but you don't want to copy more than N bytes into memory or onto disk. LimitWriter would be useful for those cases.

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Aug 19, 2022

I have also run into a variation on this theme, where something like this seemed very clean:

var b bytes.Buffer
for _, job := range jobs {
    run( job.cmd, LimitWriter(&b, job.limit))
}

because a Write error implied the job had gone haywire, and the run function could use that as a reason to kill the process. I tried LimitReader, io.Pipe things first but the elaborations to get kill-on-Write were tricky and indirect, I felt like LimitWriter really was the right abstraction.

@Fryuni
Copy link
Author

Fryuni commented Aug 22, 2022

I agree that an io.LimitWriter would be useful, but I don't think one that yields custom errors should be part of io.

Having io.LimitWriter(w io.Writer, limit int64, err error) io.Writer as proposed by @carlmjohnson would fit both use cases (production and tests) but would also require all the call sites for production code to pass the expected error, which I don't see any case where it wouldn't be io.EOF as it seems weird to me that a LimitWriter would return a different error.

Another option is to have the type public like LimitedReader:

package io

func LimitWriter(w Writer, limit int64) Writer {
	return &LimitedWriter{W: w, N: limit, Err: EOF}
}

type LimitedWriter struct {
	W Writer  // underlying writer
	N int64   // max bytes remaining
	Err error // error to be returned once limit is reached
}

func (lw *LimitedWriter) Write(p []byte) (int, error) {
	if lw.N < 1 {
		return 0, lw.Err
	}
	if lw.N < int64(len(p)) {
		p = p[:lw.N]
	}
	n, err := lw.W.Write(p)
	lw.N -= int64(n)
	return n, err
}

This would allow the same pattern as io.LimitReader and also allow tests, like the use case I proposed originally, to instantiate the structure directly:

&io.LimitedWriter{W: originalWriter, N: 100, Err: io.ErrClosedPipe}

I'd be ok with either, but IMHO it would be better to have ioutil.ErrWriter and io.LimitWriter as two separate things mirroring ioutil.ErrReader and io.LimitReader. In this case the io.LimitWriter would probably be a separate proposal.

@earthboundkid
Copy link
Contributor

I don't think io.EOF makes sense in the context of a writer. Maybe io.ErrShortWrite could be the default.

@Fryuni
Copy link
Author

Fryuni commented Aug 23, 2022

True, I was thinking of the default for LimitReader. io.ErrShortWrite makes a lot more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

7 participants