code review 6207051: go.crypto/ssh: introduce a circular buffer for chanReader

go.crypto/ssh: introduce a circular buffer for chanReader

[dave_cheney.net, 2012-05-11 21:14:38]

Hello agl@golang.org, gustav.paul@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto

[dave_cheney.net, 2012-05-11 21:28:15]

Hello agl@golang.org, gustav.paul@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-05-14 15:21:30]

LGTM.

I did think about merging the ring buffer and window tracking code. However I
come unstuck when trying to think about how to handle stdout+stderr. They are
separate data streams but share a window and that made everything messy :(

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go
File ssh/buffer.go (right):

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode16
ssh/buffer.go:16: // protects concurrent access to head, tail and eof
s/eof/closed/?

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode42
ssh/buffer.go:42: // write makes buf available for Read to receive.
this refers to `buf' but the argument is called 'b'

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode63
ssh/buffer.go:63: // Read reads data from the internal buffer in buf.
ditto about buf/b.

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode64
ssh/buffer.go:64: // Reads will block if not data is available, or until
s/not/no/

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode75
ssh/buffer.go:75: if len(buf) == 0 {
I think it's very slightly nicer if the for loop has the condition len(buf) > 0
and this condition is removed.

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode90
ssh/buffer.go:90: // out of buffers, wait for producer
this comment should be moved down to the Wait()?

[dave_cheney.net, 2012-05-15 01:12:03]

On 2012/05/14 15:21:30, agl1 wrote:
> LGTM.
> 
> I did think about merging the ring buffer and window tracking code. However I
> come unstuck when trying to think about how to handle stdout+stderr. They are
> separate data streams but share a window and that made everything messy :(

I think this is solved now that channel.remoteWin contains the windowing logic,
anything with a reference to that (which you get for free if you embed channel)
can use channel.sendWindowAdj()

[dave_cheney.net, 2012-05-15 01:12:57]

Thank you for your comments. I'm going to bang on this for a bit more to make
sure there are no conditions where channels get wedged, or worse, continue to
accept data until the process OOMs.

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go
File ssh/buffer.go (right):

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode16
ssh/buffer.go:16: // protects concurrent access to head, tail and eof
On 2012/05/14 15:21:31, agl1 wrote:
> s/eof/closed/?

Done.

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode42
ssh/buffer.go:42: // write makes buf available for Read to receive.
Sorry, i'm not sure what you mean, the receiver is b, but the argument to write
is buf.

On 2012/05/14 15:21:31, agl1 wrote:
> this refers to `buf' but the argument is called 'b'

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode63
ssh/buffer.go:63: // Read reads data from the internal buffer in buf.
See above. 

On 2012/05/14 15:21:31, agl1 wrote:
> ditto about buf/b.

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode64
ssh/buffer.go:64: // Reads will block if not data is available, or until
On 2012/05/14 15:21:31, agl1 wrote:
> s/not/no/

Done.

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode75
ssh/buffer.go:75: if len(buf) == 0 {
Thanks for the suggestion, it is nicer and the logic flows better now.

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode90
ssh/buffer.go:90: // out of buffers, wait for producer
On 2012/05/14 15:21:31, agl1 wrote:
> this comment should be moved down to the Wait()?

Done.

[dave_cheney.net, 2012-05-15 08:03:48]

Hello agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-05-15 15:30:06]

LGTM

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go
File ssh/buffer.go (right):

http://codereview.appspot.com/6207051/diff/4/ssh/buffer.go#newcode42
ssh/buffer.go:42: // write makes buf available for Read to receive.
On 2012/05/15 01:12:58, dfc wrote:
> Sorry, i'm not sure what you mean, the receiver is b, but the argument to
write
> is buf.

Sorry, my brain fart.

[dave_cheney.net, 2012-05-15 22:04:02]

*** Submitted as
http://code.google.com/p/go/source/detail?r=b24d6db9878e&repo=crypto ***

go.crypto/ssh: introduce a circular buffer for chanReader

R=agl, gustav.paul, kardianos
CC=golang-dev
http://codereview.appspot.com/6207051