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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d34a53
Return writer error if not nil in writeSequential
hmoragrega Feb 17, 2022
1c605bf
Improved test with CR feedback
hmoragrega Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions client.go
View file
Open in desktop
Expand Up @@ -1176,11 +1176,13 @@ 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.

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

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.

}
}

Expand Down
38 changes: 38 additions & 0 deletions client_integration_test.go
View file
Open in desktop
Expand Up @@ -1249,6 +1249,44 @@ func TestClientReadSequential(t *testing.T) {
}
}

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.


func TestClientWriteSequential_WriterErr(t *testing.T) {
sftp, cmd := testClient(t, READONLY, NODELAY)
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.

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.

require.NoError(t, err)

defer os.RemoveAll(d)

f, err := ioutil.TempFile(d, "write-sequential-test")
require.NoError(t, err)
fname := f.Name()
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.

f.Close()

sftpFile, err := sftp.Open(fname)
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.

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.

return 10, want
}), 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.

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.

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?

}

func TestClientReadDir(t *testing.T) {
sftp1, cmd1 := testClient(t, READONLY, NODELAY)
sftp2, cmd2 := testClientGoSvr(t, READONLY, NODELAY)
Expand Down