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

[bug] Write error not returned if reader already signals EOF #494

Closed
wants to merge 2 commits into from

Conversation

hmoragrega
Copy link
Contributor

@hmoragrega hmoragrega commented Feb 17, 2022

During the last chunk of a sequential write, when the reader already returned io.EOF, if the writer fails, the error is lost.

This happens due to err being already io.EOF so the condition to overwrite the error with the write error is false

if err == nil { // write error lost if err == io.EOF
    err = err2
}

I've provided a new test, not sure if it could be tested as a test case on another one though.

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Testing is dark, and full of terrors.

There’s nothing wrong here, just small things.

Comment on lines 1252 to 1256
type writerFunc func(b []byte) (int, error)

func (f writerFunc) Write(b []byte) (int, error) {
return f(b)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an efficient way to build mocks in Go. We’re already writing a receiver method here for a type, so we don’t need to wrap a generic function like this.

type errWriter struct{ … } with one receiver method is the same amount of code, and fewer function calls.

defer cmd.Wait()
defer sftp.Close()

sftp.disableConcurrentReads = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’ve copied code from another test that is irrelevant to the testing here. Please remove this condition as it is irrelevant noise.

Comment on lines 1272 to 1273
content := []byte("hello world")
f.Write(content)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content is not used after these two lines. Skip the temporary variable, and just pass it in directly as a function argument.

defer sftpFile.Close()

want := errors.New("error writing")
n, got := io.Copy(writerFunc(func(b []byte) (int, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not know for sure here that we’re calling into sftpFile.writeToSequential. Instead, we should just directly actually call the function itself rather than depend on io.Copy to call the expected function.


require.Error(t, got)
assert.ErrorIs(t, want, got)
assert.Equal(t, int64(10), n)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This magic number is greater than it needs to be, and specified in two places.

We want to ensure that it returns whatever short write value we return, but it should be so clearly under the number of bytes expected to be copied that we can be sure at a glance that it has done a short write, and still returned the short write values. For a value of 10 here, I’m not sure if that’s a full write with error, or supposed to be a short write.

For this, an ideal value is going to be 2. No one is going to think a write of "hello world" is supposed to return 2 bytes written.

require.NoError(t, err)
defer sftpFile.Close()

want := errors.New("error writing")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test errors should be as clearly test errors as possible. Prefer errors.New("test error"). Who knows if somewhere in the code, there might be returned an "error writing" error. But nothing should ever return a "test error" error except in test code.

defer sftp.Close()

sftp.disableConcurrentReads = true
d, err := ioutil.TempDir("", "sftptest-writesequential")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want this directory name to be as precisely narrow as possible. If we make a TestClientWriteSequential and repeat this TempDir construction, now we have tests that step on each other’s toes.

Add -writeerr to the directory name.

}), sftpFile)

require.Error(t, got)
assert.ErrorIs(t, want, got)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The appropriate scope of assertion here is that it is not io.EOF. We don’t need to test that it’s the exact same error that we returned, (which is why you’re capturing it with want := errors.New(…) because otherwise the error is an opaque pointer and errors.Is(errors.New("foo"), errors.New("foo")) == false)

We just need to know it’s not an io.EOF.

client.go Outdated
Comment on lines 1179 to 1185
m, wErr := w.Write(b[:n])
written += int64(m)

if err == nil {
err = err2
if wErr != nil {
if err == nil || err == io.EOF {
err = wErr
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to prioritize returning an error from Write, then we don’t need any of this complicated logic anymore. We can just return err here directly.

m, err := w.Write(b[:n])
written += int64(m)

if err != nil {
  return written, err
}

Copy link
Contributor Author

@hmoragrega hmoragrega Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was in doubt about this one myself, I do like the early return more too, I'll change it.


require.Error(t, got)
assert.ErrorIs(t, want, got)
assert.Equal(t, int64(10), n)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing just a single write here for written probably isn’t fully reliable. Consider, if the function were implemented as:

m, err := w.Write(b[:n])
if err != nil {
  return m, err
}
written += int64(w)
`̀ `

We might need to use `timeBobWriter` instead, with a very small chunk size?

@hmoragrega
Copy link
Contributor Author

I'll work on the test side, thanks!

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More testing gotchas. Don’t worry, this sort of thing is expected when testing is given the same attention to detail as the code itself. (Intentionally looking for bugs in the tests, etc.)

@@ -1176,11 +1176,11 @@ func (f *File) writeToSequential(w io.Writer) (written int64, err error) {
if n > 0 {
f.offset += int64(n)

m, err2 := w.Write(b[:n])
m, wErr := w.Write(b[:n])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there’s no need to rename err error. Since we now have an early return, there is no need to access the two err variables at the same time, so shadowing is no longer a problem.

func (w *lastChunkErrSequentialWriter) Write(b []byte) (int, error) {
chunkSize := len(b)
w.written += chunkSize
if w.written == w.expected {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is too strict. If we shoot past w.expected then we will never enter this loop.

}

func (w *lastChunkErrSequentialWriter) Write(b []byte) (int, error) {
chunkSize := len(b)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This alias ends up adding more characters of code than it saves. Sometimes, repetition isn’t a bad thing.

content = []byte("hello world")
shortWrite = 2
)
w := lastChunkErrSequentialWriter{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

w is only ever access by pointer. So just directly declare it with a pointer: &lastChunkErrWriter (sequential is imprecisely inaccurate in this name; it need not only be used in this test.)

Unfortunately, this test still does not ensure that the same bug mentioned before could still slip through. We’re still only ever making one call to Write() as the first Write still errors out immediately. We have to set chunkSize. We need to set MaxPacketChecked(2) so that we’re writing in say 2-byte chunks. Then we want the first to succeed, then the second return early (1), so we end up with written == 3.

In the face of all of this, I think special casing all of the functionality of the writer is appropriate. First run returns len(b), nil the second returns 1, errors.New("test error"). Then we also test to ensure that the function was called only twice.

fname := f.Name()
n, err := f.Write(content)
require.NoError(t, err)
require.Equal(t, n, len(content))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.File.Write: … Write returns a non-nil error when n != len(b).

If we already passed the NoError above, then we can rely upon the contract, that n can be known to be len(content).

f, err := ioutil.TempFile(d, "write-sequential-writeerr-test")
require.NoError(t, err)
fname := f.Name()
n, err := f.Write(content)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of which, let’s use WriteString so we can have a const for content. But also since as noted below, we don’t need to test for n != len(content) anyways, we don’t need to use content more than once either, so just use the string directly.

require.NoError(t, err)
defer sftpFile.Close()

gotWritten, gotErr := sftpFile.writeToSequential(&w)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know logically that we could not have reached this code unless err == nil. So, there’s no point in renaming err here, as the previous value is already irrelevant, so the name can be reused.

Likewise, there will now be no other gotXY, so, we can shorten that variable name to just that: got.

Comment on lines +1299 to +1300
require.NotErrorIs(t, io.EOF, gotErr)
require.Equal(t, int64(shortWrite), gotWritten)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API is confusing on this matter:
func ErrorIs(t TestingT, err error, target error, msgAndArgs ...interface{}),
but:
func Equal(t TestingT, expected interface{}, actual interface{}, msgAndArgs ...interface{})

This is one of the central reasons why I tell people to avoid using require and assert. Getting the arguments in the wrong order is way too easy to do, and without checking the documentation, who knows which one is which. (Especially, since it’s require.ErrorIs(t, actual, expect) but require.Equals(t, expect, actual) )

This is especially true, when one is coming from a language that incentivizes writing equality tests as constValue == testValue so that mistakenly only using one = will not produce an unexpectedly valid assignment, but an invalid unassignable left-hand value. In Go, you cannot put an assignment in a condition, and so there is no way to accidentally use a single = assignment like this. This idiom is probably the very reason why require.Equal orders its parameters in that order… an order that is discouraged in Go.

Instead, something like this (without the comments which are here for study, rather than for good code practice):

if got != int64(shortWrite) {
	// In the `require.Equals`, the two arguments are cast into `interface{}`,
	// which means you lose the same compile-time type safety you would normally have.
	// It’s way better to use the code the exact same as your caller would use the code,
	// with verbose `if err != nil { }` et al. included.
	
	// Note the order here, `if got != expected { … }`, and the message orders `got` then `expected`.
	t.Errorf("sftpFile.Write() = %d, but expected %d", got, shortWrite)
}

Also, we don’t want to require either of these tests anyways. We only want to assert it in this case. (Since both written and err have valid values, testing both of them every test makes sense. require is only appropriate when the test is a requirement for the remaining tests to have any meaningful result and/or not panic.

@drakkan
Copy link
Collaborator

drakkan commented Mar 3, 2022

@hmoragrega thanks for noticing this bug. I'm trying to fix the test cases as requested by @puellanivis and your commits will be merged as part of #499 once that test cases are ok

@drakkan drakkan closed this Mar 3, 2022
@puellanivis
Copy link
Collaborator

Thanks for spotting the issue and getting us started on a fix @hmoragrega

@hmoragrega
Copy link
Contributor Author

No worries! I couldn't found time to apply the latest changes these week, glad you can take it from here

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

Successfully merging this pull request may close these issues.

3 participants