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 all commits
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
6 changes: 3 additions & 3 deletions client.go
View file
Open in desktop
Expand Up @@ -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.

written += int64(m)

if err == nil {
err = err2
if wErr != nil {
return written, wErr
}
}

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

type lastChunkErrSequentialWriter struct {
expected int
written int
writtenReturn int
}

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.

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.

return w.writtenReturn, errors.New("test error")
}
return chunkSize, nil
}

func TestClientWriteSequential_WriterErr(t *testing.T) {
sftp, cmd := testClient(t, READONLY, NODELAY)
defer cmd.Wait()
defer sftp.Close()

d, err := ioutil.TempDir("", "sftptest-writesequential-writeerr")
require.NoError(t, err)

defer os.RemoveAll(d)

var (
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.

expected: len(content),
writtenReturn: shortWrite,
}

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)
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).

require.NoError(t, f.Close())

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

require.NotErrorIs(t, io.EOF, gotErr)
require.Equal(t, int64(shortWrite), gotWritten)
Comment on lines +1299 to +1300
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.

}

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