[v2] The road to an actual v2.0.0 release.

[puellanivis]

I’ve started a local branch of v1 that is wrapping the v2 implementation, to be able to exploit all the tests there against the new implementation. I’ve already found a few issues, notably in the RoughDisconnect tests. The branch to follow for the v2 fixes is the improve-v2-to-wrap-v1 branch (link to relevant PR here). This issue is going to be where I post issues that come up in this process, so that I can track what came up, and how it was resolved.

[puellanivis]

BenchmarkWrite > 32k has nearly double the data transferred

After porting the BenchmarkWrite and BenchmarkRead this issue came up:

BenchmarkWrite32k
BenchmarkWrite32k-10                     	     106	  10860741 ns/op	 965.49 MB/s	   34788 B/op	     985 allocs/op
BenchmarkWrite128k
    client_integration_test.go:434: wrong file size: want 10485883, got 20971643
--- FAIL: BenchmarkWrite128k
BenchmarkWrite512k
    client_integration_test.go:434: wrong file size: want 10485883, got 20971643
--- FAIL: BenchmarkWrite512k
BenchmarkWrite1MiB
    client_integration_test.go:434: wrong file size: want 10485883, got 20971643
--- FAIL: BenchmarkWrite1MiB
BenchmarkWrite4MiB
    client_integration_test.go:434: wrong file size: want 10485883, got 18874491
--- FAIL: BenchmarkWrite4MiB
BenchmarkWrite4MiBDelay10Msec
    client_integration_test.go:434: wrong file size: want 10485883, got 18874491
--- FAIL: BenchmarkWrite4MiBDelay10Msec
BenchmarkWrite4MiBDelay50Msec
    client_integration_test.go:434: wrong file size: want 10485883, got 18874491
--- FAIL: BenchmarkWrite4MiBDelay50Msec
BenchmarkWrite4MiBDelay150Msec
    client_integration_test.go:434: wrong file size: want 10485883, got 18874491
--- FAIL: BenchmarkWrite4MiBDelay150Msec

This clearly demonstrates that the writeatSeq is fine since the writeat function has a small read size guard: if len(b) <= f.cl.maxDataLen {. It also shows that there is an approximately doubling of the data size transferred in the non-sequential transfer!

Root-Cause

The writeat method was incrementing the f.offset in addition to returning bytes written. This caused each writeat to then start the next writeat at the wrong offset.

[drakkan]

Thank you for all your work. I’m really overloaded, but I’ll try to find some time over the next few weeks to test and review the v2 branch.

[puellanivis]

RoughDisconnect tests hang

Right from the first TestServerRoughDisconnect there were issues.

• The Client.recv function blocked forever on receiving from the channel or ctx.Done, this was made aware of the closed state of the Client, and would fall out and discard the channel in this case.
• The WorkPool implementation needed to know if it were closing or not. A disconnect and a call to Close would close the channel, and cause a send on closed channel panic. So the implementation was changed to shortcut returning the channel to the pool after the WorkPool has been closed.
• The immediately previous bug fix introduced a different bug, where the p.wg.Done was not being called in the case where the WorkPool was closed, and so this caused the WorkPool.Close to hang on the p.wg.Wait for a WaitGroup.Done that would never come.

[puellanivis]

Missing functionality

• There was no Client.Wait exposed.
• There was no GetExtension method exposed, and the unexported hasPermission while more standards compliant cannot provide the functionality that the v1 Client.HasExtension provides. However there’s also no reason to not expose the HasPermission.
• There was a ReadDirContext but no ReaddirContext.
• The v1 OpenFile does not take a perm argument, and sends an FXP_OPEN packet that has an empty attributes encoding. This has been changed so that OpenFile simply sends an empty attributes if the perm argument is 0.
• There was no RemoveAll.

[puellanivis]

rangedir returning the . and .. directory entries

The os.ReadDir and os.Readdir implementations both discard the . and ... The v2 implementation was not doing this. This caused RemoveAll to enter an infinite loop where it kept getting called on stdpath.Join(path, "."), which path.Cleans itself to path. 😐

[puellanivis]

Importing tests from v1

A number of tests make a lot of sense to import from v1 to v2, particularly where they cannot be reimplemented in v2 (like where the Client.conn writer is wrapped by a timebombWriter), or where the test is pretty straightforwardly useful even in v2, like the RoughDisconnect tests—especially where they are testing for a deadlock.

• TestClientNoSid as an example.

[lucarin91]

There was no Client.Wait exposed.

Hi, are you willing to reintroduce that? It seems an useful way to know when the sftp client disconnect for what ever reason, if not do you have some other way to handle that?

[puellanivis]

There was no Client.Wait exposed.

Hi, are you willing to reintroduce that? It seems an useful way to know when the sftp client disconnect for what ever reason, if not do you have some other way to handle that?

Yes, the functionality will be reintroduced. It was an erroneous absence. Basically, I think that, because it was on the underlying connection, rather than the client, I missed it.

[quite]

I have not looked at the v2 stuff at all, but I just wonder if it makes sense to do any work on implementing additional extensions on top of current master? Or does v2 change so much that one should target that instead? I found myself wanting support for "lsetstat@openssh.com" (to be able to do utimensat etc on a remote symlink (not its target)).

[puellanivis]

I would definitely suggest going for targeting v2 for extensions. I already have quite a few, but I hadn’t seen this lsetstat@openssh.com one yet.

Probably need to think on how I expect an extended type request gets sent. sftp.Client should probably support it through a direct LSetStat call… or rather the various chown chtimes etc should have an L prefix varient.

Though utimensat isn’t going to be possible, even lsetstat@openssh.com only has second precision through the File Attributes specification.