LogDog: Add Butler stream server package.

LogDog: Add Butler stream server package.

BUG=

Committed: https://github.com/luci/luci-go/commit/d46d171e77a7a3d3e06a70e07e97a39949153979

[dnj, 2015-11-03 18:05:37]

dnj@chromium.org changed reviewers:
+ estaab@chromium.org, hinoka@chromium.org, iannucci@chromium.org

[dnj, 2015-11-03 18:05:38]

PTAL

[dnj, 2015-11-03 20:37:42]

Review order:
(butlerproto) https://codereview.chromium.org/1321273002/
(StreamServer) https://codereview.chromium.org/1429993002/
(Bundler) https://codereview.chromium.org/1412063008/
(Output) https://codereview.chromium.org/1211053004/

[estaab, 2015-11-06 22:09:44]

Ok, here's my attempt at reviewing this code, which amounts to me asking for
clarification on parts I didn't understand.

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
File client/logdog/butlerlib/streamclient/client.go (right):

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client.go:22: ProtocolFrameHeaderMagic =
[]byte("BTLR1\x1E")
If and when will this change, and what would it change to?

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client.go:49: func (r *protocolRegistry)
newClient(path string) (Client, error) {
It's not clear from just the name and type what the format of path should be so
it probably deserves a comment. (after reading the public interface this makes
more sense, so perhaps changing ordering would help also)

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client.go:56: if f, ok :=
r.protocols[parts[0]]; ok {
Do we not need to hold a reader lock when reading from a shared map? Do we only
lock in register() because we can instantiate a new map?

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client.go:69: type Client interface {
Are there any style guidelines around ordering of public and private code in a
file? As a user I would want to see this at the top (along with New() and
NewStream()).

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
File client/logdog/butlerlib/streamclient/client_npipe_posix.go (right):

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client_npipe_posix.go:5: // +build darwin
dragonfly freebsd linux netbsd openbsd
What does this line do?

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
File client/logdog/butlerlib/streamclient/stream.go (right):

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/stream.go:17: type Stream interface {
So the caller doesn't control when opening of the stream happens?

I imagine error handling would be a little harder with all classes of errors
happening at the first call to WriteDatagram(). With an Open() all
initialization errors can be dealt with once. wdyt?

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/stream.go:35: var _ Stream =
(*streamImpl)(nil)
What does this do?

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/stream.go:59: s.rioW =
recordio.NewWriter(s.WriteCloser)
Can this fail?

[dnj (Google), 2015-11-07 16:47:40]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-11-07 16:47:42]

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
File client/logdog/butlerlib/streamclient/client.go (right):

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client.go:22: ProtocolFrameHeaderMagic =
[]byte("BTLR1\x1E")
On 2015/11/06 22:09:44, estaab wrote:
> If and when will this change, and what would it change to?

No idea - some other byte string that doesn't begin with these characters :)

This serves two purposes:
- to disambiguate a Butler stream from some happenstance string of bytes (they
probably won't start with these characters), and
- to allow an upgrade to the wire format, if one is ever needed. e.g., a switch
to something other than recordio/JSON.

So TBH probably never, but it's not a bad idea to build this in from day one.

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client.go:56: if f, ok :=
r.protocols[parts[0]]; ok {
On 2015/11/06 22:09:44, estaab wrote:
> Do we not need to hold a reader lock when reading from a shared map? Do we
only
> lock in register() because we can instantiate a new map?

As long as nothing's writing, we don't need to hold the lock to read. The design
has the services be registered on "init()", so this is safe.

However, the lock is cheap, and I don't enforce anything against runtime
protocol registration, so locking here makes sense. TBH I added the lock
last-minute once I split out the registry so I could register test protocols,
and I forgot to add it here.

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client.go:69: type Client interface {
On 2015/11/06 22:09:44, estaab wrote:
> Are there any style guidelines around ordering of public and private code in a
> file? As a user I would want to see this at the top (along with New() and
> NewStream()).

Guidelines are for readability. I split the registry into a different file.

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
File client/logdog/butlerlib/streamclient/client_npipe_posix.go (right):

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/client_npipe_posix.go:5: // +build darwin
dragonfly freebsd linux netbsd openbsd
On 2015/11/06 22:09:44, estaab wrote:
> What does this line do?

This makes it so that Go build will only include this file if the build platform
is one of the named platforms.

e.g., this file is ignored on Windows.

The Windows file has a similar feature due to it being named "_windows.go".
Unfortunately there isn't a POSIX-comprehensive version of that, so I have to
manually call out POSIX platforms.

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
File client/logdog/butlerlib/streamclient/stream.go (right):

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/stream.go:17: type Stream interface {
On 2015/11/06 22:09:44, estaab wrote:
> So the caller doesn't control when opening of the stream happens?
> 
> I imagine error handling would be a little harder with all classes of errors
> happening at the first call to WriteDatagram(). With an Open() all
> initialization errors can be dealt with once. wdyt?

Hmm?

The caller creates a new stream via "Client.New", which creates the stream and
performs the handshake.

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/stream.go:35: var _ Stream =
(*streamImpl)(nil)
On 2015/11/06 22:09:44, estaab wrote:
> What does this do?

This is a compile-time assert that the "streamImpl" class implements the Stream
interface.

https://codereview.chromium.org/1429993002/diff/20001/client/logdog/butlerlib...
client/logdog/butlerlib/streamclient/stream.go:59: s.rioW =
recordio.NewWriter(s.WriteCloser)
On 2015/11/06 22:09:44, estaab wrote:
> Can this fail?

Nope, this is a data structure initialization, so just fields populated. Usage
of the writer can fail (see below), but creation cannot*.

* Well, short of OOM.

[dnj (Google), 2015-11-07 16:49:05]

Patchset #3 (id:40001) has been deleted

[estaab, 2015-11-09 22:30:51]

lgtm

[dnj, 2015-11-10 00:47:08]

ping iannucci@ :)

[iannucci, 2015-11-12 22:44:25]

generally lgtm with comments

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/handshake.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/handshake.go:46: func
getProtocolFromMagic(ctx context.Context, r io.Reader) (handshakeProtocol,
error) {
why is this whole thing split into two functions? Why not just

  handshake(context.Context, io.Reader) (*streamproto.Properties, error)

?

Is it 'in case you have 2 protocol versions'? If that's the case then I would
strongly advocate just splitting it up when you have a second version.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/handshake.go:92: count, hr, err :=
fr.ReadFrame()
s/count/frameSize ?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/npipe.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe.go:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
can we call this filed named_pipe? I thought it was an n-way pipe at first :)

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe.go:53: // Initialize our stream
channel.
why not during initBase?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe.go:239: }
... something?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/npipe_posix.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe_posix.go:58: os.Remove(path)
error?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/npipe_windows.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe_windows.go:22: //
NewNamedPipeServer instantiates a new POSIX named pipe server instance.
POSIX?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe_windows.go:33: return
fmt.Sprintf("NamedPipeServer[%s]", s.address)
we should distinguish these String()'s by something other than the capitalized
first letter :)

Also why the String() / GoString() disparity?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
File client/logdog/butlerlib/streamclient/client.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
why not butlerlib/stream/client and butlerlib/stream/server (and
butlerlib/stream/proto)?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:26: ProtocolFrameHeaderMagic =
[]byte("BTLR1\x1E")
should be defined in butlerlib/stream and shared by client/server?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:54: //     socket at
"path/to/socket".
I assume you actually want /path/to/socket, right? Should it be required to be
absolute?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:57: //   - net.pipe:name
describes a stream server listening on Windows named pipe
net.pipe? not named.pipe?

or pipe:?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:73: passing := false
passing? handedOff?

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
File client/logdog/butlerlib/streamclient/registry.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/registry.go:65:
defaultRegistry.register(name, f)
mutex? or no need?

[dnj, 2015-11-13 20:17:15]

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/handshake.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/handshake.go:46: func
getProtocolFromMagic(ctx context.Context, r io.Reader) (handshakeProtocol,
error) {
On 2015/11/12 22:44:24, iannucci wrote:
> why is this whole thing split into two functions? Why not just
> 
>   handshake(context.Context, io.Reader) (*streamproto.Properties, error)
> 
> ?
> 
> Is it 'in case you have 2 protocol versions'? If that's the case then I would
> strongly advocate just splitting it up when you have a second version.

Essentially yes. Okay, SGTM Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/handshake.go:92: count, hr, err :=
fr.ReadFrame()
On 2015/11/12 22:44:24, iannucci wrote:
> s/count/frameSize ?

Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/npipe.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe.go:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/11/12 22:44:24, iannucci wrote:
> can we call this filed named_pipe? I thought it was an n-way pipe at first :)

Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe.go:53: // Initialize our stream
channel.
On 2015/11/12 22:44:24, iannucci wrote:
> why not during initBase?

Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe.go:239: }
On 2015/11/12 22:44:24, iannucci wrote:
> ... something?

Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/npipe_posix.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe_posix.go:58: os.Remove(path)
On 2015/11/12 22:44:24, iannucci wrote:
> error?

Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
File client/internal/logdog/butler/streamserver/npipe_windows.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe_windows.go:22: //
NewNamedPipeServer instantiates a new POSIX named pipe server instance.
On 2015/11/12 22:44:24, iannucci wrote:
> POSIX?

Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/internal/...
client/internal/logdog/butler/streamserver/npipe_windows.go:33: return
fmt.Sprintf("NamedPipeServer[%s]", s.address)
On 2015/11/12 22:44:24, iannucci wrote:
> we should distinguish these String()'s by something other than the capitalized
> first letter :)
> 
> Also why the String() / GoString() disparity?

This was before logging, so I remove this altogether now.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
File client/logdog/butlerlib/streamclient/client.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/11/12 22:44:24, iannucci wrote:
> why not butlerlib/stream/client and butlerlib/stream/server (and
> butlerlib/stream/proto)?

Mainly b/c on import, "streamclient" package is more clear than "client". Also
"streamserver" is in a separate package space (internal).

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:26: ProtocolFrameHeaderMagic =
[]byte("BTLR1\x1E")
On 2015/11/12 22:44:24, iannucci wrote:
> should be defined in butlerlib/stream and shared by client/server?

Oops, it actually already is :/

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:54: //     socket at
"path/to/socket".
On 2015/11/12 22:44:25, iannucci wrote:
> I assume you actually want /path/to/socket, right? Should it be required to be
> absolute?

Sure - I wasn't putting too much stock in the example comment, but might as
well.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:57: //   - net.pipe:name
describes a stream server listening on Windows named pipe
On 2015/11/12 22:44:24, iannucci wrote:
> net.pipe? not named.pipe?
> 
> or pipe:?

I went with "net.pipe" because it is actually the Windows term.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/client.go:73: passing := false
On 2015/11/12 22:44:24, iannucci wrote:
> passing? handedOff?

Done.

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
File client/logdog/butlerlib/streamclient/registry.go (right):

https://chromiumcodereview.appspot.com/1429993002/diff/60001/client/logdog/bu...
client/logdog/butlerlib/streamclient/registry.go:65:
defaultRegistry.register(name, f)
On 2015/11/12 22:44:25, iannucci wrote:
> mutex? or no need?

Mutex is on a per-registry basis, so not needed here.

[dnj, 2015-11-13 20:29:26]

The CQ bit was checked by dnj@chromium.org

[dnj, 2015-11-13 20:29:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org, estaab@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1429993002/#ps80001
(title: "Updated after comments, cleaned up, simplified.")

[dnj, 2015-11-13 20:29:39]

The CQ bit was unchecked by dnj@chromium.org

[commit-bot: I haz the power, 2015-11-13 20:29:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1429993002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1429993002/80001

[dnj, 2015-11-13 20:33:19]

The CQ bit was checked by dnj@chromium.org

[dnj, 2015-11-13 20:33:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org, estaab@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1429993002/#ps100001 (title: "Fix not
building on Windows.")

[commit-bot: I haz the power, 2015-11-13 20:33:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1429993002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1429993002/100001

[commit-bot: I haz the power, 2015-11-13 20:35:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-13 20:35:30]

Try jobs failed on following builders:
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)

[dnj, 2015-11-13 22:33:56]

The CQ bit was checked by dnj@chromium.org

[dnj, 2015-11-13 22:33:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org, estaab@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1429993002/#ps120001 (title: "Fix stupid
goimports error.")

[commit-bot: I haz the power, 2015-11-13 22:34:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1429993002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1429993002/120001

[dnj, 2015-11-13 22:34:37]

Description was changed from

==========
LogDog: Add Butler stream server package.

BUG=
==========

to

==========
LogDog: Add Butler stream server package.

BUG=
==========

[dnj, 2015-11-13 22:40:48]

The CQ bit was checked by dnj@chromium.org

[dnj, 2015-11-13 22:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org, estaab@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1429993002/#ps140001 (title: "Bind POSIX
test to POSIX domains.")

[commit-bot: I haz the power, 2015-11-13 22:40:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1429993002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1429993002/140001

[commit-bot: I haz the power, 2015-11-13 22:43:30]

Committed patchset #7 (id:140001) as
https://github.com/luci/luci-go/commit/d46d171e77a7a3d3e06a70e07e97a39949153979