code review 15380048: go.crypto/ssh: use a separate channel for out-of-band

go.crypto/ssh: use a separate channel for out-of-band
requests.

ChannelRequests are not synchronized with incoming data, so it
is more natural to treat them through a separate API method.

This is an API change: callers must service
channel.IncomingRequests() rather than typecasting the
channel.Read() error.

[hanwen-google, 2013-10-23 05:43:32]

Hello agl1, dfc, jpsugar@google.com (cc: golang-dev@googlegroups.com),

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

[hanwen-google, 2013-10-23 05:45:27]

By request of Dave, I'm breaking my change down in smaller bits.

This is purely for reviewing convenience, this is semi-broken, as the
pendingRequest channel never is closed (similarly, channel IDs are never removed
from ServerConn.channels.)

[jpsugar, 2013-10-23 22:41:30]

LGTM

[dave_cheney.net, 2013-10-24 20:38:36]

On 2013/10/23 22:41:30, jpsugar wrote:
> LGTM

Can you please hg mail

% hg clpatch 15380048
abort: codereview issue 15380048 is out of date: patch and recent changes
conflict (e3c12cab1b35->213a06a7ce81)

I'd also like agl to sign off on this as the Server is his baby and this is a
change to the server api.

[hanwen-google, 2013-10-24 23:39:53]

Hello agl@golang.org, dave@cheney.net, jpsugar@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[hanwen-google, 2013-10-24 23:54:44]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode45
ssh/channel.go:45: 
I have an API question:

should we put the Ack method on the Request rather than on the Channel, eg.

type Request struct {
  Name string
  Payload []byte
  WantReply bool

  ch *channel  
}

func (r *Request) Reply(ok bool, payload []byte) error {
  if !r.WantReply { /* error */ }
  return r.ch.sendMessage( .. )
}

this makes the API simpler, as it makes it more obvious when AckRequest can be
run.

[dave_cheney.net, 2013-10-25 16:48:05]

On 2013/10/24 23:54:44, hanwen-google wrote:
> https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
> File ssh/channel.go (right):
> 
> https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode45
> ssh/channel.go:45: 
> I have an API question:
> 
> should we put the Ack method on the Request rather than on the Channel, eg.
> 
> type Request struct {
>   Name string
>   Payload []byte
>   WantReply bool
> 
>   ch *channel  
> }
> 
> func (r *Request) Reply(ok bool, payload []byte) error {
>   if !r.WantReply { /* error */ }
>   return r.ch.sendMessage( .. )
> }
> 
> this makes the API simpler, as it makes it more obvious when AckRequest can be
> run.

Right now Channel is specific to ServerConns, if removing AckRequest from that
interface brings us closer to making that interface less specific to Server
connections, sounds like a good change.

[dave_cheney.net, 2013-10-25 16:48:32]

Small nit. 

Still hoping to see a comment from agl.

https://codereview.appspot.com/15380048/diff/60001/ssh/session_test.go
File ssh/session_test.go (right):

https://codereview.appspot.com/15380048/diff/60001/ssh/session_test.go#newcod...
ssh/session_test.go:645: _ = <-ch.IncomingRequests()
s/_ =//

[agl1, 2013-10-25 16:52:12]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode28
ssh/channel.go:28: // SSH connection.
This change seems to suck for a user of this package. Now I have to spin off a
goroutine to manage requests and, if I forget, the channel fills up and
everything grinds to a halt?

[hanwen-google, 2013-10-25 18:31:09]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode28
ssh/channel.go:28: // SSH connection.
On 2013/10/25 16:52:12, agl1 wrote:
> This change seems to suck for a user of this package. Now I have to spin off a
> goroutine to manage requests and, if I forget, the channel fills up and
> everything grinds to a halt?

A couple of solutions:

1. we buffer incoming requests indefinitely if nobody calls IncomingRequests()?
Then it won't come to a halt, and it will simplify most use-cases. The
disadvantage is that if the remote side sends some system specific request
("keepalive@randomvendor"), the user still needs to handle it, and there is
nothing that tells him.

2. We let the user register callbacks based on the request type string. This
lets us send failure messages to unknown WantReply requests, so we will handle
"keepalive@randomvendor" correctly. We could either do

  type RequestHandler func(incoming *Request) error
  func (c *channel) RegisterHandler(reqType string, handler RequestHandler)

or

  func (c *channel) HandleRequest(reqType string) chan *Request


3. We do nothing. This is not very practical, because as it stands you cannot
practically pass off a Channel as io.Reader / io.Writer, since things like
io.ReadFull() can't handle ChannelRequests.


I'm leaning towards #2. What do you think?




I could make it instantiate the channel on demand, ie. if nothing calls
IncomingRequests(), then we have an internal buffer which fills up.

[hanwen-google, 2013-10-25 18:37:48]

Channel is not instantiated for the client side, but they're actually
the same. Clients may send and receive requests too (but we've so far
hidden this internally).

On Fri, Oct 25, 2013 at 9:48 AM,  <dave@cheney.net> wrote:
> On 2013/10/24 23:54:44, hanwen-google wrote:
>>
>> https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
>> File ssh/channel.go (right):
>
>
>
> https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode45
>>
>> ssh/channel.go:45:
>> I have an API question:
>
>
>> should we put the Ack method on the Request rather than on the
>
> Channel, eg.
>
>> type Request struct {
>>    Name string
>>    Payload []byte
>>    WantReply bool
>
>
>>    ch *channel
>> }
>
>
>> func (r *Request) Reply(ok bool, payload []byte) error {
>>    if !r.WantReply { /* error */ }
>>    return r.ch.sendMessage( .. )
>> }
>
>
>> this makes the API simpler, as it makes it more obvious when
>
> AckRequest can be
>>
>> run.
>
>
> Right now Channel is specific to ServerConns, if removing AckRequest
> from that interface brings us closer to making that interface less
> specific to Server connections, sounds like a good change.
>
> https://codereview.appspot.com/15380048/



-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[hanwen-google, 2013-10-25 18:38:48]

Let's continue discussion in the doc I set up


https://docs.google.com/document/d/1yVmyfDxWbnIhDkk4ypNSVjV8KL_GOsfCcr-ZcDpDu...


On Fri, Oct 25, 2013 at 11:37 AM, Han-Wen Nienhuys <hanwen@google.com> wrote:
> Channel is not instantiated for the client side, but they're actually
> the same. Clients may send and receive requests too (but we've so far
> hidden this internally).
>
> On Fri, Oct 25, 2013 at 9:48 AM,  <dave@cheney.net> wrote:
>> On 2013/10/24 23:54:44, hanwen-google wrote:
>>>
>>> https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
>>> File ssh/channel.go (right):
>>
>>
>>
>> https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode45
>>>
>>> ssh/channel.go:45:
>>> I have an API question:
>>
>>
>>> should we put the Ack method on the Request rather than on the
>>
>> Channel, eg.
>>
>>> type Request struct {
>>>    Name string
>>>    Payload []byte
>>>    WantReply bool
>>
>>
>>>    ch *channel
>>> }
>>
>>
>>> func (r *Request) Reply(ok bool, payload []byte) error {
>>>    if !r.WantReply { /* error */ }
>>>    return r.ch.sendMessage( .. )
>>> }
>>
>>
>>> this makes the API simpler, as it makes it more obvious when
>>
>> AckRequest can be
>>>
>>> run.
>>
>>
>> Right now Channel is specific to ServerConns, if removing AckRequest
>> from that interface brings us closer to making that interface less
>> specific to Server connections, sounds like a good change.
>>
>> https://codereview.appspot.com/15380048/
>
>
>
> --
> Han-Wen Nienhuys
> Google Munich
> hanwen@google.com



-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[agl1, 2013-10-25 19:09:28]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode28
ssh/channel.go:28: // SSH connection.
On 2013/10/25 18:31:09, hanwen-google wrote:
> On 2013/10/25 16:52:12, agl1 wrote:
> > This change seems to suck for a user of this package. Now I have to spin off
a
> > goroutine to manage requests and, if I forget, the channel fills up and
> > everything grinds to a halt?

I see the desire to be able to pass the io.Readers off to other code, even
though it's not a case that I care about.

However, you're actually using the code for real things and I'm not so I'll take
your needs in preference. Thus I think the current code is ok.

However, this is going to break the API and, if you are following the doc that
you linked to, then it's going to continue breaking several more times in ways
that are likely to affect users. (The breakages so far have either passed me by,
or have been more minor.)

Does this call for a copy in go.crypto/ssh/new during the change, or even in an
internal repo? (I would love to use Critique for this.)

That way, the changes can be stablised before switching over and there's only a
single, big, break.

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode45
ssh/channel.go:45: 
On 2013/10/24 23:54:44, hanwen-google wrote:
> I have an API question:
> 
> should we put the Ack method on the Request rather than on the Channel, eg.
> 
> type Request struct {
>   Name string
>   Payload []byte
>   WantReply bool
> 
>   ch *channel  
> }
> 
> func (r *Request) Reply(ok bool, payload []byte) error {
>   if !r.WantReply { /* error */ }
>   return r.ch.sendMessage( .. )
> }
> 
> this makes the API simpler, as it makes it more obvious when AckRequest can be
> run.

I think this makes sense.

[hanwen-google, 2013-10-25 22:41:58]

as discussed, we'll work on the gonewssh repo for now, so I'm abandoning this
change.

[hanwen-google, 2013-10-26 06:27:08]

*** Abandoned ***