From a66f148ede8f2290c21ca7c469992e2416e8b95f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 8 Dec 2013 11:50:14 -0800 Subject: [PATCH 01/37] packer/rpc: auto-incrementable ID for endpoints --- packer/rpc/build.go | 3 ++- packer/rpc/builder.go | 2 +- packer/rpc/environment.go | 2 +- packer/rpc/hook.go | 2 +- packer/rpc/post_processor.go | 2 +- packer/rpc/provisioner.go | 2 +- packer/rpc/server.go | 44 ++++++++++++++++++++++++++---------- packer/rpc/ui.go | 13 ++++++----- packer/rpc/ui_test.go | 2 +- 9 files changed, 47 insertions(+), 25 deletions(-) diff --git a/packer/rpc/build.go b/packer/rpc/build.go index 2df03a6ab..f2f7d48ab 100644 --- a/packer/rpc/build.go +++ b/packer/rpc/build.go @@ -107,7 +107,8 @@ func (b *BuildServer) Run(args *BuildRunArgs, reply *[]string) error { return err } - artifacts, err := b.build.Run(&Ui{client}, Cache(client)) + ui := &Ui{client: client} + artifacts, err := b.build.Run(ui, Cache(client)) if err != nil { return NewBasicError(err) } diff --git a/packer/rpc/builder.go b/packer/rpc/builder.go index 42f92c4d4..0c5dfab37 100644 --- a/packer/rpc/builder.go +++ b/packer/rpc/builder.go @@ -146,7 +146,7 @@ func (b *BuilderServer) Run(args *BuilderRunArgs, reply *interface{}) error { cache := Cache(client) hook := Hook(client) - ui := &Ui{client} + ui := &Ui{client: client} artifact, responseErr := b.builder.Run(ui, hook, cache) responseAddress := "" diff --git a/packer/rpc/environment.go b/packer/rpc/environment.go index 36db72c56..aaf0db412 100644 --- a/packer/rpc/environment.go +++ b/packer/rpc/environment.go @@ -114,7 +114,7 @@ func (e *Environment) Ui() packer.Ui { panic(err) } - return &Ui{client} + return &Ui{client: client} } func (e *EnvironmentServer) Builder(name *string, reply *string) error { diff --git a/packer/rpc/hook.go b/packer/rpc/hook.go index 223b96df2..cede08302 100644 --- a/packer/rpc/hook.go +++ b/packer/rpc/hook.go @@ -51,7 +51,7 @@ func (h *HookServer) Run(args *HookRunArgs, reply *interface{}) error { return err } - if err := h.hook.Run(args.Name, &Ui{client}, Communicator(client), args.Data); err != nil { + if err := h.hook.Run(args.Name, &Ui{client: client}, Communicator(client), args.Data); err != nil { return NewBasicError(err) } diff --git a/packer/rpc/post_processor.go b/packer/rpc/post_processor.go index 0a5eaefd3..2f743eb65 100644 --- a/packer/rpc/post_processor.go +++ b/packer/rpc/post_processor.go @@ -82,7 +82,7 @@ func (p *PostProcessorServer) PostProcess(address string, reply *PostProcessorPr responseAddress := "" - artifact, keep, err := p.p.PostProcess(&Ui{client}, Artifact(client)) + artifact, keep, err := p.p.PostProcess(&Ui{client: client}, Artifact(client)) if err == nil && artifact != nil { server := rpc.NewServer() RegisterArtifact(server, artifact) diff --git a/packer/rpc/provisioner.go b/packer/rpc/provisioner.go index 4cd329d6b..a62f97d45 100644 --- a/packer/rpc/provisioner.go +++ b/packer/rpc/provisioner.go @@ -71,7 +71,7 @@ func (p *ProvisionerServer) Provision(args *ProvisionerProvisionArgs, reply *int } comm := Communicator(client) - ui := &Ui{client} + ui := &Ui{client: client} if err := p.p.Provision(ui, comm); err != nil { return NewBasicError(err) diff --git a/packer/rpc/server.go b/packer/rpc/server.go index f6098ea7a..c9154160a 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -1,73 +1,93 @@ package rpc import ( + "fmt" "github.com/mitchellh/packer/packer" "net/rpc" + "sync/atomic" ) +// This keeps track of the endpoint ID to use when registering artifacts. +var endpointId uint64 = 0 + // Registers the appropriate endpoint on an RPC server to serve an // Artifact. func RegisterArtifact(s *rpc.Server, a packer.Artifact) { - s.RegisterName("Artifact", &ArtifactServer{a}) + registerComponent(s, "Artifact", &ArtifactServer{a}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Packer Build. func RegisterBuild(s *rpc.Server, b packer.Build) { - s.RegisterName("Build", &BuildServer{b}) + registerComponent(s, "Build", &BuildServer{b}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Packer Builder. func RegisterBuilder(s *rpc.Server, b packer.Builder) { - s.RegisterName("Builder", &BuilderServer{b}) + registerComponent(s, "Builder", &BuilderServer{b}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Packer Cache. func RegisterCache(s *rpc.Server, c packer.Cache) { - s.RegisterName("Cache", &CacheServer{c}) + registerComponent(s, "Cache", &CacheServer{c}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Packer Command. func RegisterCommand(s *rpc.Server, c packer.Command) { - s.RegisterName("Command", &CommandServer{c}) + registerComponent(s, "Command", &CommandServer{c}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Packer Communicator. func RegisterCommunicator(s *rpc.Server, c packer.Communicator) { - s.RegisterName("Communicator", &CommunicatorServer{c}) + registerComponent(s, "Communicator", &CommunicatorServer{c}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Packer Environment func RegisterEnvironment(s *rpc.Server, e packer.Environment) { - s.RegisterName("Environment", &EnvironmentServer{e}) + registerComponent(s, "Environment", &EnvironmentServer{e}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Hook. -func RegisterHook(s *rpc.Server, hook packer.Hook) { - s.RegisterName("Hook", &HookServer{hook}) +func RegisterHook(s *rpc.Server, h packer.Hook) { + registerComponent(s, "Hook", &HookServer{h}, false) } // Registers the appropriate endpoing on an RPC server to serve a // PostProcessor. func RegisterPostProcessor(s *rpc.Server, p packer.PostProcessor) { - s.RegisterName("PostProcessor", &PostProcessorServer{p}) + registerComponent(s, "PostProcessor", &PostProcessorServer{p}, false) } // Registers the appropriate endpoint on an RPC server to serve a packer.Provisioner func RegisterProvisioner(s *rpc.Server, p packer.Provisioner) { - s.RegisterName("Provisioner", &ProvisionerServer{p}) + registerComponent(s, "Provisioner", &ProvisionerServer{p}, false) } // Registers the appropriate endpoint on an RPC server to serve a // Packer UI func RegisterUi(s *rpc.Server, ui packer.Ui) { - s.RegisterName("Ui", &UiServer{ui}) + registerComponent(s, "Ui", &UiServer{ui}, false) +} + +// registerComponent registers a single Packer RPC component onto +// the RPC server. If id is true, then a unique ID number will be appended +// onto the end of the endpoint. +// +// The endpoint name is returned. +func registerComponent(s *rpc.Server, name string, rcvr interface{}, id bool) string { + endpoint := name + if id { + fmt.Sprintf("%s.%d", endpoint, atomic.AddUint64(&endpointId, 1)) + } + + s.RegisterName(endpoint, rcvr) + return endpoint } func serveSingleConn(s *rpc.Server) string { diff --git a/packer/rpc/ui.go b/packer/rpc/ui.go index 4d7ccc57f..1857e4928 100644 --- a/packer/rpc/ui.go +++ b/packer/rpc/ui.go @@ -9,7 +9,8 @@ import ( // An implementation of packer.Ui where the Ui is actually executed // over an RPC connection. type Ui struct { - client *rpc.Client + client *rpc.Client + endpoint string } // UiServer wraps a packer.Ui implementation and makes it exportable @@ -25,12 +26,12 @@ type UiMachineArgs struct { } func (u *Ui) Ask(query string) (result string, err error) { - err = u.client.Call("Ui.Ask", query, &result) + err = u.client.Call(u.endpoint+".Ask", query, &result) return } func (u *Ui) Error(message string) { - if err := u.client.Call("Ui.Error", message, new(interface{})); err != nil { + if err := u.client.Call(u.endpoint+".Error", message, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } @@ -41,19 +42,19 @@ func (u *Ui) Machine(t string, args ...string) { Args: args, } - if err := u.client.Call("Ui.Machine", rpcArgs, new(interface{})); err != nil { + if err := u.client.Call(u.endpoint+".Machine", rpcArgs, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } func (u *Ui) Message(message string) { - if err := u.client.Call("Ui.Message", message, new(interface{})); err != nil { + if err := u.client.Call(u.endpoint+".Message", message, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } func (u *Ui) Say(message string) { - if err := u.client.Call("Ui.Say", message, new(interface{})); err != nil { + if err := u.client.Call(u.endpoint+".Say", message, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } diff --git a/packer/rpc/ui_test.go b/packer/rpc/ui_test.go index 8a85c5573..3a46bf02c 100644 --- a/packer/rpc/ui_test.go +++ b/packer/rpc/ui_test.go @@ -62,7 +62,7 @@ func TestUiRPC(t *testing.T) { panic(err) } - uiClient := &Ui{client} + uiClient := &Ui{client: client, endpoint: "Ui"} // Basic error and say tests result, err := uiClient.Ask("query") From fe46093bcf354295ca77150fb612d87d1219f6cf Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 8 Dec 2013 18:20:27 -0800 Subject: [PATCH 02/37] packer/rpc: a muxconn... --- packer/rpc/muxconn.go | 132 +++++++++++++++++++++++++++++++++++++ packer/rpc/muxconn_test.go | 105 +++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+) create mode 100644 packer/rpc/muxconn.go create mode 100644 packer/rpc/muxconn_test.go diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go new file mode 100644 index 000000000..b1f1df2e5 --- /dev/null +++ b/packer/rpc/muxconn.go @@ -0,0 +1,132 @@ +package rpc + +import ( + "encoding/binary" + "fmt" + "io" + "log" + "sync" +) + +// MuxConn is a connection that can be used bi-directionally for RPC. Normally, +// Go RPC only allows client-to-server connections. This allows the client +// to actually act as a server as well. +// +// MuxConn works using a fairly dumb multiplexing technique of simply +// prefixing each message with whether it is on stream 0 (the original) +// or stream 1 (the client "server"). +// +// This can likely be abstracted to N streams, but by choosing only two +// we decided to cut a lot of corners and make this easily usable for Packer. +type MuxConn struct { + rwc io.ReadWriteCloser + streams map[byte]io.Writer + mu sync.RWMutex + wlock sync.Mutex +} + +func NewMuxConn(rwc io.ReadWriteCloser) *MuxConn { + m := &MuxConn{ + rwc: rwc, + streams: make(map[byte]io.Writer), + } + + go m.loop() + + return m +} + +// Stream returns a io.ReadWriteCloser that will only read/write to the +// given stream ID. No handshake is done so if the remote end does not +// have a stream open with the same ID, then the messages will simply +// be dropped. +// +// This is one of those cases where we cut corners. Since Packer only does +// local connections, we can assume that both ends are ready at a certain +// point. In a real muxer, we'd probably want a handshake here. +func (m *MuxConn) Stream(id byte) (io.ReadWriteCloser, error) { + m.mu.Lock() + defer m.mu.Unlock() + + if _, ok := m.streams[id]; ok { + return nil, fmt.Errorf("Stream %d already exists", id) + } + + // Create the stream object and channel where data will be sent to + dataR, dataW := io.Pipe() + stream := &Stream{ + id: id, + mux: m, + reader: dataR, + } + + // Set the data channel so we can write to it. + m.streams[id] = dataW + + return stream, nil +} + +func (m *MuxConn) loop() { + for { + var id byte + var length int32 + + if err := binary.Read(m.rwc, binary.BigEndian, &id); err != nil { + log.Printf("[ERR] Error reading stream ID: %s", err) + return + } + if err := binary.Read(m.rwc, binary.BigEndian, &length); err != nil { + log.Printf("[ERR] Error reading length: %s", err) + return + } + + // TODO(mitchellh): probably would be better to re-use a buffer... + data := make([]byte, length) + if _, err := m.rwc.Read(data); err != nil { + log.Printf("[ERR] Error reading data: %s", err) + return + } + + m.mu.RLock() + w, ok := m.streams[id] + if ok { + // Note that if this blocks, it'll block the whole read loop. + // Danger here... not sure how to handle it though. + w.Write(data) + } + m.mu.RUnlock() + } +} + +func (m *MuxConn) write(id byte, p []byte) (int, error) { + m.wlock.Lock() + defer m.wlock.Unlock() + + if err := binary.Write(m.rwc, binary.BigEndian, id); err != nil { + return 0, err + } + if err := binary.Write(m.rwc, binary.BigEndian, int32(len(p))); err != nil { + return 0, err + } + return m.rwc.Write(p) +} + +// Stream is a single stream of data and implements io.ReadWriteCloser +type Stream struct { + id byte + mux *MuxConn + reader io.Reader +} + +func (s *Stream) Close() error { + // Not functional yet, does it ever have to be? + return nil +} + +func (s *Stream) Read(p []byte) (int, error) { + return s.reader.Read(p) +} + +func (s *Stream) Write(p []byte) (int, error) { + return s.mux.write(s.id, p) +} diff --git a/packer/rpc/muxconn_test.go b/packer/rpc/muxconn_test.go new file mode 100644 index 000000000..4493e1c5f --- /dev/null +++ b/packer/rpc/muxconn_test.go @@ -0,0 +1,105 @@ +package rpc + +import ( + "io" + "net" + "sync" + "testing" +) + +func readStream(t *testing.T, s io.Reader) string { + var data [1024]byte + n, err := s.Read(data[:]) + if err != nil { + t.Fatalf("err: %s", err) + } + + return string(data[0:n]) +} + +func TestMuxConn(t *testing.T) { + l, err := net.Listen("tcp", ":0") + if err != nil { + t.Fatalf("err: %s", err) + } + + // When the server is done + doneCh := make(chan struct{}) + readyCh := make(chan struct{}) + + // The server side + go func() { + defer close(doneCh) + conn, err := l.Accept() + l.Close() + if err != nil { + t.Fatalf("err: %s", err) + } + defer conn.Close() + + mux := NewMuxConn(conn) + s0, err := mux.Stream(0) + if err != nil { + t.Fatalf("err: %s", err) + } + + s1, err := mux.Stream(1) + if err != nil { + t.Fatalf("err: %s", err) + } + + close(readyCh) + + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + data := readStream(t, s1) + if data != "another" { + t.Fatalf("bad: %#v", data) + } + }() + + go func() { + defer wg.Done() + data := readStream(t, s0) + if data != "hello" { + t.Fatalf("bad: %#v", data) + } + }() + + wg.Wait() + }() + + // Client side + conn, err := net.Dial("tcp", l.Addr().String()) + if err != nil { + t.Fatalf("err: %s", err) + } + defer conn.Close() + + mux := NewMuxConn(conn) + s0, err := mux.Stream(0) + if err != nil { + t.Fatalf("err: %s", err) + } + + s1, err := mux.Stream(1) + if err != nil { + t.Fatalf("err: %s", err) + } + + // Wait for the server to be ready + <-readyCh + + if _, err := s0.Write([]byte("hello")); err != nil { + t.Fatalf("err: %s", err) + } + if _, err := s1.Write([]byte("another")); err != nil { + t.Fatalf("err: %s", err) + } + + // Wait for the server to be done + <-doneCh +} From 5c6831080cd906759b1d348a0f8936d5230826a6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 8 Dec 2013 18:30:29 -0800 Subject: [PATCH 03/37] packer/rpc: close the streams when the underlying rwc closes --- packer/rpc/muxconn.go | 25 ++++++++-- packer/rpc/muxconn_test.go | 100 +++++++++++++++++++++++++++++-------- 2 files changed, 101 insertions(+), 24 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index b1f1df2e5..91b9b08d0 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -13,14 +13,14 @@ import ( // to actually act as a server as well. // // MuxConn works using a fairly dumb multiplexing technique of simply -// prefixing each message with whether it is on stream 0 (the original) -// or stream 1 (the client "server"). +// prefixing each message with what stream it is on along with the length +// of the data. // // This can likely be abstracted to N streams, but by choosing only two // we decided to cut a lot of corners and make this easily usable for Packer. type MuxConn struct { rwc io.ReadWriteCloser - streams map[byte]io.Writer + streams map[byte]io.WriteCloser mu sync.RWMutex wlock sync.Mutex } @@ -28,7 +28,7 @@ type MuxConn struct { func NewMuxConn(rwc io.ReadWriteCloser) *MuxConn { m := &MuxConn{ rwc: rwc, - streams: make(map[byte]io.Writer), + streams: make(map[byte]io.WriteCloser), } go m.loop() @@ -36,6 +36,21 @@ func NewMuxConn(rwc io.ReadWriteCloser) *MuxConn { return m } +// Close closes the underlying io.ReadWriteCloser. This will also close +// all streams that are open. +func (m *MuxConn) Close() error { + m.mu.Lock() + defer m.mu.Unlock() + + // Close all the streams + for _, w := range m.streams { + w.Close() + } + m.streams = make(map[byte]io.WriteCloser) + + return m.rwc.Close() +} + // Stream returns a io.ReadWriteCloser that will only read/write to the // given stream ID. No handshake is done so if the remote end does not // have a stream open with the same ID, then the messages will simply @@ -67,6 +82,8 @@ func (m *MuxConn) Stream(id byte) (io.ReadWriteCloser, error) { } func (m *MuxConn) loop() { + defer m.Close() + for { var id byte var length int32 diff --git a/packer/rpc/muxconn_test.go b/packer/rpc/muxconn_test.go index 4493e1c5f..9c7b69eb7 100644 --- a/packer/rpc/muxconn_test.go +++ b/packer/rpc/muxconn_test.go @@ -17,12 +17,43 @@ func readStream(t *testing.T, s io.Reader) string { return string(data[0:n]) } -func TestMuxConn(t *testing.T) { +func testMux(t *testing.T) (client *MuxConn, server *MuxConn) { l, err := net.Listen("tcp", ":0") if err != nil { t.Fatalf("err: %s", err) } + // Server side + doneCh := make(chan struct{}) + go func() { + defer close(doneCh) + conn, err := l.Accept() + l.Close() + if err != nil { + t.Fatalf("err: %s", err) + } + + server = NewMuxConn(conn) + }() + + // Client side + conn, err := net.Dial("tcp", l.Addr().String()) + if err != nil { + t.Fatalf("err: %s", err) + } + client = NewMuxConn(conn) + + // Wait for the server + <-doneCh + + return +} + +func TestMuxConn(t *testing.T) { + client, server := testMux(t) + defer client.Close() + defer server.Close() + // When the server is done doneCh := make(chan struct{}) readyCh := make(chan struct{}) @@ -30,20 +61,13 @@ func TestMuxConn(t *testing.T) { // The server side go func() { defer close(doneCh) - conn, err := l.Accept() - l.Close() - if err != nil { - t.Fatalf("err: %s", err) - } - defer conn.Close() - mux := NewMuxConn(conn) - s0, err := mux.Stream(0) + s0, err := server.Stream(0) if err != nil { t.Fatalf("err: %s", err) } - s1, err := mux.Stream(1) + s1, err := server.Stream(1) if err != nil { t.Fatalf("err: %s", err) } @@ -72,20 +96,12 @@ func TestMuxConn(t *testing.T) { wg.Wait() }() - // Client side - conn, err := net.Dial("tcp", l.Addr().String()) - if err != nil { - t.Fatalf("err: %s", err) - } - defer conn.Close() - - mux := NewMuxConn(conn) - s0, err := mux.Stream(0) + s0, err := client.Stream(0) if err != nil { t.Fatalf("err: %s", err) } - s1, err := mux.Stream(1) + s1, err := client.Stream(1) if err != nil { t.Fatalf("err: %s", err) } @@ -103,3 +119,47 @@ func TestMuxConn(t *testing.T) { // Wait for the server to be done <-doneCh } + +func TestMuxConn_clientClosesStreams(t *testing.T) { + client, server := testMux(t) + defer client.Close() + defer server.Close() + + s0, err := client.Stream(0) + if err != nil { + t.Fatalf("err: %s", err) + } + + if err := client.Close(); err != nil { + t.Fatalf("err: %s", err) + } + + // This should block forever since we never write onto this stream. + var data [1024]byte + _, err = s0.Read(data[:]) + if err != io.EOF { + t.Fatalf("err: %s", err) + } +} + +func TestMuxConn_serverClosesStreams(t *testing.T) { + client, server := testMux(t) + defer client.Close() + defer server.Close() + + s0, err := client.Stream(0) + if err != nil { + t.Fatalf("err: %s", err) + } + + if err := server.Close(); err != nil { + t.Fatalf("err: %s", err) + } + + // This should block forever since we never write onto this stream. + var data [1024]byte + _, err = s0.Read(data[:]) + if err != io.EOF { + t.Fatalf("err: %s", err) + } +} From 36a47f5b5952d7d1fe92679e869869fef2768ade Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 8 Dec 2013 18:39:14 -0800 Subject: [PATCH 04/37] packer/rpc: more fine grained lock access on MuxConn --- packer/rpc/muxconn.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 91b9b08d0..9907ce102 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -61,23 +61,27 @@ func (m *MuxConn) Close() error { // point. In a real muxer, we'd probably want a handshake here. func (m *MuxConn) Stream(id byte) (io.ReadWriteCloser, error) { m.mu.Lock() - defer m.mu.Unlock() if _, ok := m.streams[id]; ok { + m.mu.Unlock() return nil, fmt.Errorf("Stream %d already exists", id) } // Create the stream object and channel where data will be sent to dataR, dataW := io.Pipe() + + // Set the data channel so we can write to it. + m.streams[id] = dataW + + // Unlock the lock so that the reader can access the stream writer. + m.mu.Unlock() + stream := &Stream{ id: id, mux: m, reader: dataR, } - // Set the data channel so we can write to it. - m.streams[id] = dataW - return stream, nil } From 50cfb6786393e36bd2048f6b4a5445fdbb1e1840 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 14:24:55 -0800 Subject: [PATCH 05/37] packer/rpc: muxconn is a lot more sane, acts like bsd socket --- packer/rpc/artifact_test.go | 17 +-- packer/rpc/client.go | 38 ++++++ packer/rpc/client_test.go | 9 ++ packer/rpc/muxconn.go | 250 ++++++++++++++++++++++++++++++------ packer/rpc/muxconn_test.go | 20 ++- packer/rpc/server.go | 9 -- packer/rpc/server_new.go | 61 +++++++++ packer/rpc/ui.go | 13 +- packer/rpc/ui_test.go | 2 +- 9 files changed, 338 insertions(+), 81 deletions(-) create mode 100644 packer/rpc/client.go create mode 100644 packer/rpc/client_test.go create mode 100644 packer/rpc/server_new.go diff --git a/packer/rpc/artifact_test.go b/packer/rpc/artifact_test.go index 336fa6d8b..4cc49df83 100644 --- a/packer/rpc/artifact_test.go +++ b/packer/rpc/artifact_test.go @@ -2,7 +2,6 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "testing" ) @@ -31,19 +30,13 @@ func (testArtifact) Destroy() error { func TestArtifactRPC(t *testing.T) { // Create the interface to test - a := new(testArtifact) + a := new(packer.MockArtifact) // Start the server - server := rpc.NewServer() - RegisterArtifact(server, a) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - aClient := Artifact(client) + server := NewServer() + server.RegisterArtifact(a) + client := testClient(t, server) + aClient := client.Artifact() // Test if aClient.BuilderId() != "bid" { diff --git a/packer/rpc/client.go b/packer/rpc/client.go new file mode 100644 index 000000000..73b3ce1d8 --- /dev/null +++ b/packer/rpc/client.go @@ -0,0 +1,38 @@ +package rpc + +import ( + "github.com/mitchellh/packer/packer" + "io" + "net/rpc" +) + +// Client is the client end that communicates with a Packer RPC server. +// Establishing a connection is up to the user, the Client can just +// communicate over any ReadWriteCloser. +type Client struct { + mux *MuxConn + client *rpc.Client +} + +func NewClient(rwc io.ReadWriteCloser) (*Client, error) { + // Create the MuxConn around the RWC and get the client to server stream. + // This is the primary stream that we use to communicate with the + // remote RPC server. On the remote side Server.ServeConn also listens + // on this stream ID. + mux := NewMuxConn(rwc) + stream, err := mux.Dial(0) + if err != nil { + return nil, err + } + + return &Client{ + mux: mux, + client: rpc.NewClient(stream), + }, nil +} + +func (c *Client) Artifact() packer.Artifact { + return &artifact{ + client: c.client, + } +} diff --git a/packer/rpc/client_test.go b/packer/rpc/client_test.go new file mode 100644 index 000000000..1f9b930a7 --- /dev/null +++ b/packer/rpc/client_test.go @@ -0,0 +1,9 @@ +package rpc + +import ( + "testing" +) + +func testClient(t *testing.T, server *Server) *Client { + return nil +} diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 9907ce102..cf296be04 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -6,6 +6,7 @@ import ( "io" "log" "sync" + "time" ) // MuxConn is a connection that can be used bi-directionally for RPC. Normally, @@ -20,15 +21,24 @@ import ( // we decided to cut a lot of corners and make this easily usable for Packer. type MuxConn struct { rwc io.ReadWriteCloser - streams map[byte]io.WriteCloser + streams map[byte]*Stream mu sync.RWMutex wlock sync.Mutex } +type muxPacketType byte + +const ( + muxPacketSyn muxPacketType = iota + muxPacketAck + muxPacketFin + muxPacketData +) + func NewMuxConn(rwc io.ReadWriteCloser) *MuxConn { m := &MuxConn{ rwc: rwc, - streams: make(map[byte]io.WriteCloser), + streams: make(map[byte]*Stream), } go m.loop() @@ -46,56 +56,140 @@ func (m *MuxConn) Close() error { for _, w := range m.streams { w.Close() } - m.streams = make(map[byte]io.WriteCloser) + m.streams = make(map[byte]*Stream) return m.rwc.Close() } -// Stream returns a io.ReadWriteCloser that will only read/write to the -// given stream ID. No handshake is done so if the remote end does not -// have a stream open with the same ID, then the messages will simply -// be dropped. -// -// This is one of those cases where we cut corners. Since Packer only does -// local connections, we can assume that both ends are ready at a certain -// point. In a real muxer, we'd probably want a handshake here. -func (m *MuxConn) Stream(id byte) (io.ReadWriteCloser, error) { - m.mu.Lock() +// Accept accepts a multiplexed connection with the given ID. This +// will block until a request is made to connect. +func (m *MuxConn) Accept(id byte) (io.ReadWriteCloser, error) { + stream, err := m.openStream(id) + if err != nil { + return nil, err + } - if _, ok := m.streams[id]; ok { - m.mu.Unlock() - return nil, fmt.Errorf("Stream %d already exists", id) + // If the stream isn't closed, then it is already open somehow + stream.mu.Lock() + if stream.state != streamStateSynRecv && stream.state != streamStateClosed { + stream.mu.Unlock() + return nil, fmt.Errorf("Stream already open in bad state: %d", stream.state) + } + + if stream.state == streamStateSynRecv { + // Fast track establishing since we already got the syn + stream.setState(streamStateEstablished) + stream.mu.Unlock() + } + + if stream.state != streamStateEstablished { + // Go into the listening state + stream.setState(streamStateListen) + stream.mu.Unlock() + + // Wait for the connection to establish + ACCEPT_ESTABLISH_LOOP: + for { + time.Sleep(50 * time.Millisecond) + stream.mu.Lock() + switch stream.state { + case streamStateListen: + stream.mu.Unlock() + case streamStateEstablished: + stream.mu.Unlock() + break ACCEPT_ESTABLISH_LOOP + default: + defer stream.mu.Unlock() + return nil, fmt.Errorf("Stream went to bad state: %d", stream.state) + } + } + } + + // Send the ack down + if _, err := m.write(stream.id, muxPacketAck, nil); err != nil { + return nil, err + } + + return stream, nil +} + +// Dial opens a connection to the remote end using the given stream ID. +// An Accept on the remote end will only work with if the IDs match. +func (m *MuxConn) Dial(id byte) (io.ReadWriteCloser, error) { + stream, err := m.openStream(id) + if err != nil { + return nil, err + } + + // If the stream isn't closed, then it is already open somehow + stream.mu.Lock() + if stream.state != streamStateClosed { + stream.mu.Unlock() + return nil, fmt.Errorf("Stream already open in bad state: %d", stream.state) + } + + // Open a connection + if _, err := m.write(stream.id, muxPacketSyn, nil); err != nil { + return nil, err + } + stream.setState(streamStateSynSent) + stream.mu.Unlock() + + for { + time.Sleep(50 * time.Millisecond) + stream.mu.Lock() + switch stream.state { + case streamStateSynSent: + stream.mu.Unlock() + case streamStateEstablished: + stream.mu.Unlock() + return stream, nil + default: + defer stream.mu.Unlock() + return nil, fmt.Errorf("Stream went to bad state: %d", stream.state) + } + } +} + +func (m *MuxConn) openStream(id byte) (*Stream, error) { + m.mu.Lock() + defer m.mu.Unlock() + + if stream, ok := m.streams[id]; ok { + return stream, nil } // Create the stream object and channel where data will be sent to dataR, dataW := io.Pipe() // Set the data channel so we can write to it. - m.streams[id] = dataW - - // Unlock the lock so that the reader can access the stream writer. - m.mu.Unlock() - stream := &Stream{ id: id, mux: m, reader: dataR, + writer: dataW, } + stream.setState(streamStateClosed) - return stream, nil + m.streams[id] = stream + return m.streams[id], nil } func (m *MuxConn) loop() { defer m.Close() + var id byte + var packetType muxPacketType + var length int32 for { - var id byte - var length int32 - if err := binary.Read(m.rwc, binary.BigEndian, &id); err != nil { log.Printf("[ERR] Error reading stream ID: %s", err) return } + if err := binary.Read(m.rwc, binary.BigEndian, &packetType); err != nil { + log.Printf("[ERR] Error reading packet type: %s", err) + return + } if err := binary.Read(m.rwc, binary.BigEndian, &length); err != nil { log.Printf("[ERR] Error reading length: %s", err) return @@ -103,44 +197,115 @@ func (m *MuxConn) loop() { // TODO(mitchellh): probably would be better to re-use a buffer... data := make([]byte, length) - if _, err := m.rwc.Read(data); err != nil { - log.Printf("[ERR] Error reading data: %s", err) + if length > 0 { + if _, err := m.rwc.Read(data); err != nil { + log.Printf("[ERR] Error reading data: %s", err) + return + } + } + + stream, err := m.openStream(id) + if err != nil { + log.Printf("[ERR] Error opening stream %d: %s", id, err) return } - m.mu.RLock() - w, ok := m.streams[id] - if ok { - // Note that if this blocks, it'll block the whole read loop. - // Danger here... not sure how to handle it though. - w.Write(data) + switch packetType { + case muxPacketAck: + stream.mu.Lock() + if stream.state == streamStateSynSent { + stream.setState(streamStateEstablished) + } else { + log.Printf("[ERR] Ack received for stream in state: %d", stream.state) + } + stream.mu.Unlock() + case muxPacketSyn: + stream.mu.Lock() + switch stream.state { + case streamStateClosed: + stream.setState(streamStateSynRecv) + case streamStateListen: + stream.setState(streamStateEstablished) + default: + log.Printf("[ERR] Syn received for stream in state: %d", stream.state) + } + stream.mu.Unlock() + case muxPacketFin: + stream.mu.Lock() + stream.setState(streamStateClosed) + stream.writer.Close() + stream.mu.Unlock() + + m.mu.Lock() + delete(m.streams, stream.id) + m.mu.Unlock() + case muxPacketData: + stream.mu.Lock() + if stream.state == streamStateEstablished { + stream.writer.Write(data) + } else { + log.Printf("[ERR] Data received for stream in state: %d", stream.state) + } + stream.mu.Unlock() } - m.mu.RUnlock() } } -func (m *MuxConn) write(id byte, p []byte) (int, error) { +func (m *MuxConn) write(id byte, dataType muxPacketType, p []byte) (int, error) { m.wlock.Lock() defer m.wlock.Unlock() if err := binary.Write(m.rwc, binary.BigEndian, id); err != nil { return 0, err } + if err := binary.Write(m.rwc, binary.BigEndian, byte(dataType)); err != nil { + return 0, err + } if err := binary.Write(m.rwc, binary.BigEndian, int32(len(p))); err != nil { return 0, err } + if len(p) == 0 { + return 0, nil + } return m.rwc.Write(p) } // Stream is a single stream of data and implements io.ReadWriteCloser type Stream struct { - id byte - mux *MuxConn - reader io.Reader + id byte + mux *MuxConn + reader io.Reader + writer io.WriteCloser + state streamState + stateUpdated time.Time + mu sync.Mutex } +type streamState byte + +const ( + streamStateClosed streamState = iota + streamStateListen + streamStateSynRecv + streamStateSynSent + streamStateEstablished + streamStateFinWait +) + func (s *Stream) Close() error { - // Not functional yet, does it ever have to be? + s.mu.Lock() + defer s.mu.Unlock() + + if s.state != streamStateEstablished { + return fmt.Errorf("Stream in bad state: %d", s.state) + } + + if _, err := s.mux.write(s.id, muxPacketFin, nil); err != nil { + return err + } + + s.setState(streamStateClosed) + s.writer.Close() return nil } @@ -149,5 +314,10 @@ func (s *Stream) Read(p []byte) (int, error) { } func (s *Stream) Write(p []byte) (int, error) { - return s.mux.write(s.id, p) + return s.mux.write(s.id, muxPacketData, p) +} + +func (s *Stream) setState(state streamState) { + s.state = state + s.stateUpdated = time.Now().UTC() } diff --git a/packer/rpc/muxconn_test.go b/packer/rpc/muxconn_test.go index 9c7b69eb7..fce29b3af 100644 --- a/packer/rpc/muxconn_test.go +++ b/packer/rpc/muxconn_test.go @@ -56,24 +56,21 @@ func TestMuxConn(t *testing.T) { // When the server is done doneCh := make(chan struct{}) - readyCh := make(chan struct{}) // The server side go func() { defer close(doneCh) - s0, err := server.Stream(0) + s0, err := server.Accept(0) if err != nil { t.Fatalf("err: %s", err) } - s1, err := server.Stream(1) + s1, err := server.Dial(1) if err != nil { t.Fatalf("err: %s", err) } - close(readyCh) - var wg sync.WaitGroup wg.Add(2) @@ -96,19 +93,16 @@ func TestMuxConn(t *testing.T) { wg.Wait() }() - s0, err := client.Stream(0) + s0, err := client.Dial(0) if err != nil { t.Fatalf("err: %s", err) } - s1, err := client.Stream(1) + s1, err := client.Accept(1) if err != nil { t.Fatalf("err: %s", err) } - // Wait for the server to be ready - <-readyCh - if _, err := s0.Write([]byte("hello")); err != nil { t.Fatalf("err: %s", err) } @@ -124,8 +118,9 @@ func TestMuxConn_clientClosesStreams(t *testing.T) { client, server := testMux(t) defer client.Close() defer server.Close() + go server.Accept(0) - s0, err := client.Stream(0) + s0, err := client.Dial(0) if err != nil { t.Fatalf("err: %s", err) } @@ -146,8 +141,9 @@ func TestMuxConn_serverClosesStreams(t *testing.T) { client, server := testMux(t) defer client.Close() defer server.Close() + go server.Accept(0) - s0, err := client.Stream(0) + s0, err := client.Dial(0) if err != nil { t.Fatalf("err: %s", err) } diff --git a/packer/rpc/server.go b/packer/rpc/server.go index c9154160a..e1ab19663 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -1,15 +1,10 @@ package rpc import ( - "fmt" "github.com/mitchellh/packer/packer" "net/rpc" - "sync/atomic" ) -// This keeps track of the endpoint ID to use when registering artifacts. -var endpointId uint64 = 0 - // Registers the appropriate endpoint on an RPC server to serve an // Artifact. func RegisterArtifact(s *rpc.Server, a packer.Artifact) { @@ -82,10 +77,6 @@ func RegisterUi(s *rpc.Server, ui packer.Ui) { // The endpoint name is returned. func registerComponent(s *rpc.Server, name string, rcvr interface{}, id bool) string { endpoint := name - if id { - fmt.Sprintf("%s.%d", endpoint, atomic.AddUint64(&endpointId, 1)) - } - s.RegisterName(endpoint, rcvr) return endpoint } diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go new file mode 100644 index 000000000..4fcda1f00 --- /dev/null +++ b/packer/rpc/server_new.go @@ -0,0 +1,61 @@ +package rpc + +import ( + "fmt" + "github.com/mitchellh/packer/packer" + "io" + "log" + "net/rpc" + "sync/atomic" +) + +// Server represents an RPC server for Packer. This must be paired on +// the other side with a Client. +type Server struct { + endpointId uint64 + rpcServer *rpc.Server +} + +// NewServer returns a new Packer RPC server. +func NewServer() *Server { + return &Server{ + endpointId: 0, + rpcServer: rpc.NewServer(), + } +} + +func (s *Server) RegisterArtifact(a packer.Artifact) { + s.registerComponent("Artifact", &ArtifactServer{a}, false) +} + +// ServeConn serves a single connection over the RPC server. It is up +// to the caller to obtain a proper io.ReadWriteCloser. +func (s *Server) ServeConn(conn io.ReadWriteCloser) { + mux := NewMuxConn(conn) + defer mux.Close() + + // Get stream ID 0, which we always use as the stream for serving + // our RPC server on. + stream, err := mux.Accept(0) + if err != nil { + log.Printf("[ERR] Error retrieving stream for serving: %s", err) + return + } + + s.rpcServer.ServeConn(stream) +} + +// registerComponent registers a single Packer RPC component onto +// the RPC server. If id is true, then a unique ID number will be appended +// onto the end of the endpoint. +// +// The endpoint name is returned. +func (s *Server) registerComponent(name string, rcvr interface{}, id bool) string { + endpoint := name + if id { + fmt.Sprintf("%s.%d", endpoint, atomic.AddUint64(&s.endpointId, 1)) + } + + s.rpcServer.RegisterName(endpoint, rcvr) + return endpoint +} diff --git a/packer/rpc/ui.go b/packer/rpc/ui.go index 1857e4928..4d7ccc57f 100644 --- a/packer/rpc/ui.go +++ b/packer/rpc/ui.go @@ -9,8 +9,7 @@ import ( // An implementation of packer.Ui where the Ui is actually executed // over an RPC connection. type Ui struct { - client *rpc.Client - endpoint string + client *rpc.Client } // UiServer wraps a packer.Ui implementation and makes it exportable @@ -26,12 +25,12 @@ type UiMachineArgs struct { } func (u *Ui) Ask(query string) (result string, err error) { - err = u.client.Call(u.endpoint+".Ask", query, &result) + err = u.client.Call("Ui.Ask", query, &result) return } func (u *Ui) Error(message string) { - if err := u.client.Call(u.endpoint+".Error", message, new(interface{})); err != nil { + if err := u.client.Call("Ui.Error", message, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } @@ -42,19 +41,19 @@ func (u *Ui) Machine(t string, args ...string) { Args: args, } - if err := u.client.Call(u.endpoint+".Machine", rpcArgs, new(interface{})); err != nil { + if err := u.client.Call("Ui.Machine", rpcArgs, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } func (u *Ui) Message(message string) { - if err := u.client.Call(u.endpoint+".Message", message, new(interface{})); err != nil { + if err := u.client.Call("Ui.Message", message, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } func (u *Ui) Say(message string) { - if err := u.client.Call(u.endpoint+".Say", message, new(interface{})); err != nil { + if err := u.client.Call("Ui.Say", message, new(interface{})); err != nil { log.Printf("Error in Ui RPC call: %s", err) } } diff --git a/packer/rpc/ui_test.go b/packer/rpc/ui_test.go index 3a46bf02c..5241f7989 100644 --- a/packer/rpc/ui_test.go +++ b/packer/rpc/ui_test.go @@ -62,7 +62,7 @@ func TestUiRPC(t *testing.T) { panic(err) } - uiClient := &Ui{client: client, endpoint: "Ui"} + uiClient := &Ui{client: client} // Basic error and say tests result, err := uiClient.Ask("query") From 61fd3f733326bda553a065dfd9bea7669c53f86e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 14:29:28 -0800 Subject: [PATCH 06/37] packer/rpc: update docs --- packer/rpc/muxconn.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index cf296be04..ee34b515f 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -14,11 +14,9 @@ import ( // to actually act as a server as well. // // MuxConn works using a fairly dumb multiplexing technique of simply -// prefixing each message with what stream it is on along with the length -// of the data. -// -// This can likely be abstracted to N streams, but by choosing only two -// we decided to cut a lot of corners and make this easily usable for Packer. +// framing every piece of data sent into a prefix + data format. Streams +// are established using a subset of the TCP protocol. Only a subset is +// necessary since we assume ordering on the underlying RWC. type MuxConn struct { rwc io.ReadWriteCloser streams map[byte]*Stream From 105e5f6a6d8a63cf09a4a2cb8d88dbf8d312d523 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 14:44:26 -0800 Subject: [PATCH 07/37] packer/rpc: tests passing --- packer/rpc/client_test.go | 26 +++++++++++++++++++++++++- packer/rpc/server_new.go | 4 ++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/packer/rpc/client_test.go b/packer/rpc/client_test.go index 1f9b930a7..09976aabb 100644 --- a/packer/rpc/client_test.go +++ b/packer/rpc/client_test.go @@ -1,9 +1,33 @@ package rpc import ( + "net" "testing" ) func testClient(t *testing.T, server *Server) *Client { - return nil + l, err := net.Listen("tcp", ":0") + if err != nil { + t.Fatalf("err: %s", err) + } + + go func() { + conn, err := l.Accept() + if err != nil { + t.Fatalf("err: %s", err) + } + server.ServeConn(conn) + }() + + clientConn, err := net.Dial("tcp", l.Addr().String()) + if err != nil { + t.Fatalf("err: %s", err) + } + + client, err := NewClient(clientConn) + if err != nil { + t.Fatalf("err: %s", err) + } + + return client } diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 4fcda1f00..9ad95b6ce 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -34,8 +34,8 @@ func (s *Server) ServeConn(conn io.ReadWriteCloser) { mux := NewMuxConn(conn) defer mux.Close() - // Get stream ID 0, which we always use as the stream for serving - // our RPC server on. + // Accept a connection on stream ID 0, which is always used for + // normal client to server connections. stream, err := mux.Accept(0) if err != nil { log.Printf("[ERR] Error retrieving stream for serving: %s", err) From e9f7a1418c5be8a24b82f04e0fffe5a9e5db85c2 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 14:46:33 -0800 Subject: [PATCH 08/37] packer/rpc: use packer.MockArtifact --- packer/rpc/artifact_test.go | 22 ---------------------- packer/rpc/build_test.go | 2 +- packer/rpc/builder_test.go | 2 +- packer/rpc/post_processor_test.go | 4 ++-- 4 files changed, 4 insertions(+), 26 deletions(-) diff --git a/packer/rpc/artifact_test.go b/packer/rpc/artifact_test.go index 4cc49df83..1cefe082f 100644 --- a/packer/rpc/artifact_test.go +++ b/packer/rpc/artifact_test.go @@ -6,28 +6,6 @@ import ( "testing" ) -type testArtifact struct{} - -func (testArtifact) BuilderId() string { - return "bid" -} - -func (testArtifact) Files() []string { - return []string{"a", "b"} -} - -func (testArtifact) Id() string { - return "id" -} - -func (testArtifact) String() string { - return "string" -} - -func (testArtifact) Destroy() error { - return nil -} - func TestArtifactRPC(t *testing.T) { // Create the interface to test a := new(packer.MockArtifact) diff --git a/packer/rpc/build_test.go b/packer/rpc/build_test.go index 3cb5e1fb2..60c2c0111 100644 --- a/packer/rpc/build_test.go +++ b/packer/rpc/build_test.go @@ -8,7 +8,7 @@ import ( "testing" ) -var testBuildArtifact = &testArtifact{} +var testBuildArtifact = &packer.MockArtifact{} type testBuild struct { nameCalled bool diff --git a/packer/rpc/builder_test.go b/packer/rpc/builder_test.go index fa20a4b85..81e65a0d1 100644 --- a/packer/rpc/builder_test.go +++ b/packer/rpc/builder_test.go @@ -7,7 +7,7 @@ import ( "testing" ) -var testBuilderArtifact = &testArtifact{} +var testBuilderArtifact = &packer.MockArtifact{} func builderRPCClient(t *testing.T) (*packer.MockBuilder, packer.Builder) { b := new(packer.MockBuilder) diff --git a/packer/rpc/post_processor_test.go b/packer/rpc/post_processor_test.go index 75f3deca0..4f0ae871e 100644 --- a/packer/rpc/post_processor_test.go +++ b/packer/rpc/post_processor_test.go @@ -7,7 +7,7 @@ import ( "testing" ) -var testPostProcessorArtifact = new(testArtifact) +var testPostProcessorArtifact = new(packer.MockArtifact) type TestPostProcessor struct { configCalled bool @@ -62,7 +62,7 @@ func TestPostProcessorRPC(t *testing.T) { } // Test PostProcess - a := new(testArtifact) + a := new(packer.MockArtifact) ui := new(testUi) artifact, _, err := pClient.PostProcess(ui, a) if err != nil { From 984dd224f3b9af7a1ac5ff65d2938e021cfd552e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 14:51:13 -0800 Subject: [PATCH 09/37] packer/rpc: Cache --- packer/rpc/cache_test.go | 24 +++++++++--------------- packer/rpc/client.go | 14 ++++++++++++++ packer/rpc/server_new.go | 4 ++++ 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/packer/rpc/cache_test.go b/packer/rpc/cache_test.go index 46cb81d08..3fce1ee4a 100644 --- a/packer/rpc/cache_test.go +++ b/packer/rpc/cache_test.go @@ -2,7 +2,6 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "testing" ) @@ -52,19 +51,14 @@ func TestCacheRPC(t *testing.T) { c := new(testCache) // Start the server - server := rpc.NewServer() - RegisterCache(server, c) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - rpcClient, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("bad: %s", err) - } - client := Cache(rpcClient) + server := NewServer() + server.RegisterCache(c) + client := testClient(t, server) + defer client.Close() + cacheClient := client.Cache() // Test Lock - client.Lock("foo") + cacheClient.Lock("foo") if !c.lockCalled { t.Fatal("should be called") } @@ -73,7 +67,7 @@ func TestCacheRPC(t *testing.T) { } // Test Unlock - client.Unlock("foo") + cacheClient.Unlock("foo") if !c.unlockCalled { t.Fatal("should be called") } @@ -82,7 +76,7 @@ func TestCacheRPC(t *testing.T) { } // Test RLock - client.RLock("foo") + cacheClient.RLock("foo") if !c.rlockCalled { t.Fatal("should be called") } @@ -91,7 +85,7 @@ func TestCacheRPC(t *testing.T) { } // Test RUnlock - client.RUnlock("foo") + cacheClient.RUnlock("foo") if !c.runlockCalled { t.Fatal("should be called") } diff --git a/packer/rpc/client.go b/packer/rpc/client.go index 73b3ce1d8..6f4bc80b2 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -31,8 +31,22 @@ func NewClient(rwc io.ReadWriteCloser) (*Client, error) { }, nil } +func (c *Client) Close() error { + if err := c.client.Close(); err != nil { + return err + } + + return c.mux.Close() +} + func (c *Client) Artifact() packer.Artifact { return &artifact{ client: c.client, } } + +func (c *Client) Cache() packer.Cache { + return &cache{ + client: c.client, + } +} diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 9ad95b6ce..0adcfce5b 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -28,6 +28,10 @@ func (s *Server) RegisterArtifact(a packer.Artifact) { s.registerComponent("Artifact", &ArtifactServer{a}, false) } +func (s *Server) RegisterCache(c packer.Cache) { + s.registerComponent("Cache", &CacheServer{c}, false) +} + // ServeConn serves a single connection over the RPC server. It is up // to the caller to obtain a proper io.ReadWriteCloser. func (s *Server) ServeConn(conn io.ReadWriteCloser) { From a32cd59c2920ebfb8056ec4467aa4118e9e9e633 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 14:57:18 -0800 Subject: [PATCH 10/37] packer/rpc: PostProcessor --- packer/rpc/client.go | 6 ++++++ packer/rpc/post_processor_test.go | 20 +++++++------------- packer/rpc/server_new.go | 4 ++++ 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/packer/rpc/client.go b/packer/rpc/client.go index 6f4bc80b2..d80ce2899 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -50,3 +50,9 @@ func (c *Client) Cache() packer.Cache { client: c.client, } } + +func (c *Client) PostProcessor() packer.PostProcessor { + return &postProcessor{ + client: c.client, + } +} diff --git a/packer/rpc/post_processor_test.go b/packer/rpc/post_processor_test.go index 4f0ae871e..990e9a8ce 100644 --- a/packer/rpc/post_processor_test.go +++ b/packer/rpc/post_processor_test.go @@ -2,7 +2,6 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "testing" ) @@ -35,20 +34,15 @@ func TestPostProcessorRPC(t *testing.T) { p := new(TestPostProcessor) // Start the server - server := rpc.NewServer() - RegisterPostProcessor(server, p) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("Error connecting to rpc: %s", err) - } + server := NewServer() + server.RegisterPostProcessor(p) + client := testClient(t, server) + defer client.Close() + ppClient := client.PostProcessor() // Test Configure config := 42 - pClient := PostProcessor(client) - err = pClient.Configure(config) + err := ppClient.Configure(config) if err != nil { t.Fatalf("error: %s", err) } @@ -64,7 +58,7 @@ func TestPostProcessorRPC(t *testing.T) { // Test PostProcess a := new(packer.MockArtifact) ui := new(testUi) - artifact, _, err := pClient.PostProcess(ui, a) + artifact, _, err := ppClient.PostProcess(ui, a) if err != nil { t.Fatalf("err: %s", err) } diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 0adcfce5b..6b596bc11 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -32,6 +32,10 @@ func (s *Server) RegisterCache(c packer.Cache) { s.registerComponent("Cache", &CacheServer{c}, false) } +func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { + s.registerComponent("PostProcessor", &PostProcessorServer{p}, false) +} + // ServeConn serves a single connection over the RPC server. It is up // to the caller to obtain a proper io.ReadWriteCloser. func (s *Server) ServeConn(conn io.ReadWriteCloser) { From 4ba5c2ef46ab1d13fe25df95aabaf5f4f283e075 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 15:44:00 -0800 Subject: [PATCH 11/37] packer/rpc: post-processors work on a single connection --- packer/rpc/artifact.go | 15 +++--- packer/rpc/client.go | 23 +++++++-- packer/rpc/post_processor.go | 78 +++++++++++++++++++------------ packer/rpc/post_processor_test.go | 10 ++-- packer/rpc/server.go | 13 +----- packer/rpc/server_new.go | 61 +++++++++++++++++++----- packer/rpc/ui.go | 3 +- 7 files changed, 136 insertions(+), 67 deletions(-) diff --git a/packer/rpc/artifact.go b/packer/rpc/artifact.go index d71f8831a..8ca3ef869 100644 --- a/packer/rpc/artifact.go +++ b/packer/rpc/artifact.go @@ -8,7 +8,8 @@ import ( // An implementation of packer.Artifact where the artifact is actually // available over an RPC connection. type artifact struct { - client *rpc.Client + client *rpc.Client + endpoint string } // ArtifactServer wraps a packer.Artifact implementation and makes it @@ -18,32 +19,32 @@ type ArtifactServer struct { } func Artifact(client *rpc.Client) *artifact { - return &artifact{client} + return &artifact{client: client} } func (a *artifact) BuilderId() (result string) { - a.client.Call("Artifact.BuilderId", new(interface{}), &result) + a.client.Call(a.endpoint+".BuilderId", new(interface{}), &result) return } func (a *artifact) Files() (result []string) { - a.client.Call("Artifact.Files", new(interface{}), &result) + a.client.Call(a.endpoint+".Files", new(interface{}), &result) return } func (a *artifact) Id() (result string) { - a.client.Call("Artifact.Id", new(interface{}), &result) + a.client.Call(a.endpoint+".Id", new(interface{}), &result) return } func (a *artifact) String() (result string) { - a.client.Call("Artifact.String", new(interface{}), &result) + a.client.Call(a.endpoint+".String", new(interface{}), &result) return } func (a *artifact) Destroy() error { var result error - if err := a.client.Call("Artifact.Destroy", new(interface{}), &result); err != nil { + if err := a.client.Call(a.endpoint+".Destroy", new(interface{}), &result); err != nil { return err } diff --git a/packer/rpc/client.go b/packer/rpc/client.go index d80ce2899..08ddeee12 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -12,6 +12,7 @@ import ( type Client struct { mux *MuxConn client *rpc.Client + server *rpc.Server } func NewClient(rwc io.ReadWriteCloser) (*Client, error) { @@ -20,14 +21,28 @@ func NewClient(rwc io.ReadWriteCloser) (*Client, error) { // remote RPC server. On the remote side Server.ServeConn also listens // on this stream ID. mux := NewMuxConn(rwc) - stream, err := mux.Dial(0) + clientConn, err := mux.Dial(0) if err != nil { + mux.Close() return nil, err } + // Accept connection ID 1 which is what the remote end uses to + // be an RPC client back to us so we can even serve some objects. + serverConn, err := mux.Accept(1) + if err != nil { + mux.Close() + return nil, err + } + + // Start our RPC server on this end + server := rpc.NewServer() + go server.ServeConn(serverConn) + return &Client{ mux: mux, - client: rpc.NewClient(stream), + client: rpc.NewClient(clientConn), + server: server, }, nil } @@ -41,7 +56,8 @@ func (c *Client) Close() error { func (c *Client) Artifact() packer.Artifact { return &artifact{ - client: c.client, + client: c.client, + endpoint: DefaultArtifactEndpoint, } } @@ -54,5 +70,6 @@ func (c *Client) Cache() packer.Cache { func (c *Client) PostProcessor() packer.PostProcessor { return &postProcessor{ client: c.client, + server: c.server, } } diff --git a/packer/rpc/post_processor.go b/packer/rpc/post_processor.go index 2f743eb65..20e4f9b70 100644 --- a/packer/rpc/post_processor.go +++ b/packer/rpc/post_processor.go @@ -9,27 +9,36 @@ import ( // executed over an RPC connection. type postProcessor struct { client *rpc.Client + server *rpc.Server } // PostProcessorServer wraps a packer.PostProcessor implementation and makes it // exportable as part of a Golang RPC server. type PostProcessorServer struct { - p packer.PostProcessor + client *rpc.Client + server *rpc.Server + p packer.PostProcessor } type PostProcessorConfigureArgs struct { Configs []interface{} } +type PostProcessorPostProcessArgs struct { + ArtifactEndpoint string + UiEndpoint string +} + type PostProcessorProcessResponse struct { - Err error - Keep bool - RPCAddress string + Err error + Keep bool + ArtifactEndpoint string } func PostProcessor(client *rpc.Client) *postProcessor { - return &postProcessor{client} + return &postProcessor{client: client} } + func (p *postProcessor) Configure(raw ...interface{}) (err error) { args := &PostProcessorConfigureArgs{Configs: raw} if cerr := p.client.Call("PostProcessor.Configure", args, &err); cerr != nil { @@ -40,12 +49,21 @@ func (p *postProcessor) Configure(raw ...interface{}) (err error) { } func (p *postProcessor) PostProcess(ui packer.Ui, a packer.Artifact) (packer.Artifact, bool, error) { - server := rpc.NewServer() - RegisterArtifact(server, a) - RegisterUi(server, ui) + artifactEndpoint := registerComponent(p.server, "Artifact", &ArtifactServer{ + artifact: a, + }, true) + + uiEndpoint := registerComponent(p.server, "Ui", &UiServer{ + ui: ui, + }, true) + + args := PostProcessorPostProcessArgs{ + ArtifactEndpoint: artifactEndpoint, + UiEndpoint: uiEndpoint, + } var response PostProcessorProcessResponse - if err := p.client.Call("PostProcessor.PostProcess", serveSingleConn(server), &response); err != nil { + if err := p.client.Call("PostProcessor.PostProcess", &args, &response); err != nil { return nil, false, err } @@ -53,16 +71,14 @@ func (p *postProcessor) PostProcess(ui packer.Ui, a packer.Artifact) (packer.Art return nil, false, response.Err } - if response.RPCAddress == "" { + if response.ArtifactEndpoint == "" { return nil, false, nil } - client, err := rpcDial(response.RPCAddress) - if err != nil { - return nil, false, err - } - - return Artifact(client), response.Keep, nil + return &artifact{ + client: p.client, + endpoint: response.ArtifactEndpoint, + }, response.Keep, nil } func (p *PostProcessorServer) Configure(args *PostProcessorConfigureArgs, reply *error) error { @@ -74,19 +90,23 @@ func (p *PostProcessorServer) Configure(args *PostProcessorConfigureArgs, reply return nil } -func (p *PostProcessorServer) PostProcess(address string, reply *PostProcessorProcessResponse) error { - client, err := rpcDial(address) - if err != nil { - return err +func (p *PostProcessorServer) PostProcess(args *PostProcessorPostProcessArgs, reply *PostProcessorProcessResponse) error { + artifact := &artifact{ + client: p.client, + endpoint: args.ArtifactEndpoint, } - responseAddress := "" + ui := &Ui{ + client: p.client, + endpoint: args.UiEndpoint, + } - artifact, keep, err := p.p.PostProcess(&Ui{client: client}, Artifact(client)) - if err == nil && artifact != nil { - server := rpc.NewServer() - RegisterArtifact(server, artifact) - responseAddress = serveSingleConn(server) + var artifactEndpoint string + artifactResult, keep, err := p.p.PostProcess(ui, artifact) + if err == nil && artifactResult != nil { + artifactEndpoint = registerComponent(p.server, "Artifact", &ArtifactServer{ + artifact: artifactResult, + }, true) } if err != nil { @@ -94,9 +114,9 @@ func (p *PostProcessorServer) PostProcess(address string, reply *PostProcessorPr } *reply = PostProcessorProcessResponse{ - Err: err, - Keep: keep, - RPCAddress: responseAddress, + Err: err, + Keep: keep, + ArtifactEndpoint: artifactEndpoint, } return nil diff --git a/packer/rpc/post_processor_test.go b/packer/rpc/post_processor_test.go index 990e9a8ce..e22136ed6 100644 --- a/packer/rpc/post_processor_test.go +++ b/packer/rpc/post_processor_test.go @@ -56,7 +56,9 @@ func TestPostProcessorRPC(t *testing.T) { } // Test PostProcess - a := new(packer.MockArtifact) + a := &packer.MockArtifact{ + IdValue: "ppTestId", + } ui := new(testUi) artifact, _, err := ppClient.PostProcess(ui, a) if err != nil { @@ -67,12 +69,12 @@ func TestPostProcessorRPC(t *testing.T) { t.Fatal("postprocess should be called") } - if p.ppArtifact.BuilderId() != "bid" { + if p.ppArtifact.Id() != "ppTestId" { t.Fatal("unknown artifact") } - if artifact.BuilderId() != "bid" { - t.Fatal("unknown result artifact") + if artifact.Id() != "id" { + t.Fatalf("unknown artifact: %s", artifact.Id()) } } diff --git a/packer/rpc/server.go b/packer/rpc/server.go index e1ab19663..68116885b 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -56,7 +56,7 @@ func RegisterHook(s *rpc.Server, h packer.Hook) { // Registers the appropriate endpoing on an RPC server to serve a // PostProcessor. func RegisterPostProcessor(s *rpc.Server, p packer.PostProcessor) { - registerComponent(s, "PostProcessor", &PostProcessorServer{p}, false) + registerComponent(s, "PostProcessor", &PostProcessorServer{p: p}, false) } // Registers the appropriate endpoint on an RPC server to serve a packer.Provisioner @@ -70,17 +70,6 @@ func RegisterUi(s *rpc.Server, ui packer.Ui) { registerComponent(s, "Ui", &UiServer{ui}, false) } -// registerComponent registers a single Packer RPC component onto -// the RPC server. If id is true, then a unique ID number will be appended -// onto the end of the endpoint. -// -// The endpoint name is returned. -func registerComponent(s *rpc.Server, name string, rcvr interface{}, id bool) string { - endpoint := name - s.RegisterName(endpoint, rcvr) - return endpoint -} - func serveSingleConn(s *rpc.Server) string { l := netListenerInRange(portRangeMin, portRangeMax) diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 6b596bc11..a4782a0d1 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -9,31 +9,35 @@ import ( "sync/atomic" ) +var endpointId uint64 + +const ( + DefaultArtifactEndpoint string = "Artifact" +) + // Server represents an RPC server for Packer. This must be paired on // the other side with a Client. type Server struct { - endpointId uint64 - rpcServer *rpc.Server + components map[string]interface{} } // NewServer returns a new Packer RPC server. func NewServer() *Server { return &Server{ - endpointId: 0, - rpcServer: rpc.NewServer(), + components: make(map[string]interface{}), } } func (s *Server) RegisterArtifact(a packer.Artifact) { - s.registerComponent("Artifact", &ArtifactServer{a}, false) + s.components[DefaultArtifactEndpoint] = a } func (s *Server) RegisterCache(c packer.Cache) { - s.registerComponent("Cache", &CacheServer{c}, false) + s.components["Cache"] = c } func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { - s.registerComponent("PostProcessor", &PostProcessorServer{p}, false) + s.components["PostProcessor"] = p } // ServeConn serves a single connection over the RPC server. It is up @@ -50,7 +54,42 @@ func (s *Server) ServeConn(conn io.ReadWriteCloser) { return } - s.rpcServer.ServeConn(stream) + clientConn, err := mux.Dial(1) + if err != nil { + log.Printf("[ERR] Error connecting to client stream: %s", err) + return + } + client := rpc.NewClient(clientConn) + + // Create the RPC server + server := rpc.NewServer() + for endpoint, iface := range s.components { + var endpointVal interface{} + + switch v := iface.(type) { + case packer.Artifact: + endpointVal = &ArtifactServer{ + artifact: v, + } + case packer.Cache: + endpointVal = &CacheServer{ + cache: v, + } + case packer.PostProcessor: + endpointVal = &PostProcessorServer{ + client: client, + server: server, + p: v, + } + default: + log.Printf("[ERR] Unknown component for endpoint: %s", endpoint) + return + } + + registerComponent(server, endpoint, endpointVal, false) + } + + server.ServeConn(stream) } // registerComponent registers a single Packer RPC component onto @@ -58,12 +97,12 @@ func (s *Server) ServeConn(conn io.ReadWriteCloser) { // onto the end of the endpoint. // // The endpoint name is returned. -func (s *Server) registerComponent(name string, rcvr interface{}, id bool) string { +func registerComponent(server *rpc.Server, name string, rcvr interface{}, id bool) string { endpoint := name if id { - fmt.Sprintf("%s.%d", endpoint, atomic.AddUint64(&s.endpointId, 1)) + fmt.Sprintf("%s.%d", endpoint, atomic.AddUint64(&endpointId, 1)) } - s.rpcServer.RegisterName(endpoint, rcvr) + server.RegisterName(endpoint, rcvr) return endpoint } diff --git a/packer/rpc/ui.go b/packer/rpc/ui.go index 4d7ccc57f..da032160a 100644 --- a/packer/rpc/ui.go +++ b/packer/rpc/ui.go @@ -9,7 +9,8 @@ import ( // An implementation of packer.Ui where the Ui is actually executed // over an RPC connection. type Ui struct { - client *rpc.Client + client *rpc.Client + endpoint string } // UiServer wraps a packer.Ui implementation and makes it exportable From 171781c3c618bb8710f93073e01e734dc47a34f7 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 16:22:11 -0800 Subject: [PATCH 12/37] packer/rpc: work-in-progress commit --- packer/rpc/artifact_test.go | 6 ++- packer/rpc/cache_test.go | 7 +-- packer/rpc/client.go | 22 +++------ packer/rpc/client_test.go | 23 +++++++-- packer/rpc/post_processor_test.go | 7 +-- packer/rpc/server_new.go | 80 +++++++++++++------------------ packer/rpc/ui_test.go | 16 ++----- 7 files changed, 75 insertions(+), 86 deletions(-) diff --git a/packer/rpc/artifact_test.go b/packer/rpc/artifact_test.go index 1cefe082f..d5405ab6e 100644 --- a/packer/rpc/artifact_test.go +++ b/packer/rpc/artifact_test.go @@ -11,9 +11,11 @@ func TestArtifactRPC(t *testing.T) { a := new(packer.MockArtifact) // Start the server - server := NewServer() + client, server := testClientServer(t) + defer client.Close() + defer server.Close() server.RegisterArtifact(a) - client := testClient(t, server) + aClient := client.Artifact() // Test diff --git a/packer/rpc/cache_test.go b/packer/rpc/cache_test.go index 3fce1ee4a..a9a1f110f 100644 --- a/packer/rpc/cache_test.go +++ b/packer/rpc/cache_test.go @@ -51,10 +51,11 @@ func TestCacheRPC(t *testing.T) { c := new(testCache) // Start the server - server := NewServer() - server.RegisterCache(c) - client := testClient(t, server) + client, server := testClientServer(t) defer client.Close() + defer server.Close() + server.RegisterCache(c) + cacheClient := client.Cache() // Test Lock diff --git a/packer/rpc/client.go b/packer/rpc/client.go index 08ddeee12..99f0b296d 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -12,7 +12,6 @@ import ( type Client struct { mux *MuxConn client *rpc.Client - server *rpc.Server } func NewClient(rwc io.ReadWriteCloser) (*Client, error) { @@ -27,22 +26,9 @@ func NewClient(rwc io.ReadWriteCloser) (*Client, error) { return nil, err } - // Accept connection ID 1 which is what the remote end uses to - // be an RPC client back to us so we can even serve some objects. - serverConn, err := mux.Accept(1) - if err != nil { - mux.Close() - return nil, err - } - - // Start our RPC server on this end - server := rpc.NewServer() - go server.ServeConn(serverConn) - return &Client{ mux: mux, client: rpc.NewClient(clientConn), - server: server, }, nil } @@ -70,6 +56,12 @@ func (c *Client) Cache() packer.Cache { func (c *Client) PostProcessor() packer.PostProcessor { return &postProcessor{ client: c.client, - server: c.server, + } +} + +func (c *Client) Ui() packer.Ui { + return &Ui{ + client: c.client, + endpoint: DefaultUiEndpoint, } } diff --git a/packer/rpc/client_test.go b/packer/rpc/client_test.go index 09976aabb..c0595cbe4 100644 --- a/packer/rpc/client_test.go +++ b/packer/rpc/client_test.go @@ -5,29 +5,44 @@ import ( "testing" ) -func testClient(t *testing.T, server *Server) *Client { +func testConn(t *testing.T) (net.Conn, net.Conn) { l, err := net.Listen("tcp", ":0") if err != nil { t.Fatalf("err: %s", err) } + var serverConn net.Conn + doneCh := make(chan struct{}) go func() { - conn, err := l.Accept() + defer close(doneCh) + defer l.Close() + var err error + serverConn, err = l.Accept() if err != nil { t.Fatalf("err: %s", err) } - server.ServeConn(conn) }() clientConn, err := net.Dial("tcp", l.Addr().String()) if err != nil { t.Fatalf("err: %s", err) } + <-doneCh + + return clientConn, serverConn +} + +func testClientServer(t *testing.T) (*Client, *Server) { + clientConn, serverConn := testConn(t) + + server := NewServer(serverConn) + go server.Serve() client, err := NewClient(clientConn) if err != nil { + server.Close() t.Fatalf("err: %s", err) } - return client + return client, server } diff --git a/packer/rpc/post_processor_test.go b/packer/rpc/post_processor_test.go index e22136ed6..418889ac5 100644 --- a/packer/rpc/post_processor_test.go +++ b/packer/rpc/post_processor_test.go @@ -34,10 +34,11 @@ func TestPostProcessorRPC(t *testing.T) { p := new(TestPostProcessor) // Start the server - server := NewServer() - server.RegisterPostProcessor(p) - client := testClient(t, server) + client, server := testClientServer(t) defer client.Close() + defer server.Close() + server.RegisterPostProcessor(p) + ppClient := client.PostProcessor() // Test Configure diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index a4782a0d1..afed8fe58 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -12,84 +12,68 @@ import ( var endpointId uint64 const ( - DefaultArtifactEndpoint string = "Artifact" + DefaultArtifactEndpoint string = "Artifact" + DefaultCacheEndpoint = "Cache" + DefaultPostProcessorEndpoint = "PostProcessor" + DefaultUiEndpoint = "Ui" ) // Server represents an RPC server for Packer. This must be paired on // the other side with a Client. type Server struct { - components map[string]interface{} + mux *MuxConn + server *rpc.Server } // NewServer returns a new Packer RPC server. -func NewServer() *Server { +func NewServer(conn io.ReadWriteCloser) *Server { return &Server{ - components: make(map[string]interface{}), + mux: NewMuxConn(conn), + server: rpc.NewServer(), } } +func (s *Server) Close() error { + return s.mux.Close() +} + func (s *Server) RegisterArtifact(a packer.Artifact) { - s.components[DefaultArtifactEndpoint] = a + s.server.RegisterName(DefaultArtifactEndpoint, &ArtifactServer{ + artifact: a, + }) } func (s *Server) RegisterCache(c packer.Cache) { - s.components["Cache"] = c + s.server.RegisterName(DefaultCacheEndpoint, &CacheServer{ + cache: c, + }) } func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { - s.components["PostProcessor"] = p + s.server.RegisterName(DefaultPostProcessorEndpoint, &PostProcessorServer{ + p: p, + }) +} + +func (s *Server) RegisterUi(ui packer.Ui) { + s.server.RegisterName(DefaultUiEndpoint, &UiServer{ + ui: ui, + }) } // ServeConn serves a single connection over the RPC server. It is up // to the caller to obtain a proper io.ReadWriteCloser. -func (s *Server) ServeConn(conn io.ReadWriteCloser) { - mux := NewMuxConn(conn) - defer mux.Close() - +func (s *Server) Serve() { // Accept a connection on stream ID 0, which is always used for // normal client to server connections. - stream, err := mux.Accept(0) + stream, err := s.mux.Accept(0) + defer stream.Close() if err != nil { log.Printf("[ERR] Error retrieving stream for serving: %s", err) return } - clientConn, err := mux.Dial(1) - if err != nil { - log.Printf("[ERR] Error connecting to client stream: %s", err) - return - } - client := rpc.NewClient(clientConn) - - // Create the RPC server - server := rpc.NewServer() - for endpoint, iface := range s.components { - var endpointVal interface{} - - switch v := iface.(type) { - case packer.Artifact: - endpointVal = &ArtifactServer{ - artifact: v, - } - case packer.Cache: - endpointVal = &CacheServer{ - cache: v, - } - case packer.PostProcessor: - endpointVal = &PostProcessorServer{ - client: client, - server: server, - p: v, - } - default: - log.Printf("[ERR] Unknown component for endpoint: %s", endpoint) - return - } - - registerComponent(server, endpoint, endpointVal, false) - } - - server.ServeConn(stream) + s.server.ServeConn(stream) } // registerComponent registers a single Packer RPC component onto diff --git a/packer/rpc/ui_test.go b/packer/rpc/ui_test.go index 5241f7989..4dd2d7036 100644 --- a/packer/rpc/ui_test.go +++ b/packer/rpc/ui_test.go @@ -1,7 +1,6 @@ package rpc import ( - "net/rpc" "reflect" "testing" ) @@ -52,17 +51,12 @@ func TestUiRPC(t *testing.T) { ui := new(testUi) // Start the RPC server - server := rpc.NewServer() - RegisterUi(server, ui) - address := serveSingleConn(server) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterUi(ui) - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - panic(err) - } - - uiClient := &Ui{client: client} + uiClient := client.Ui() // Basic error and say tests result, err := uiClient.Ask("query") From ec68a3fd39f15e9b8f1a92b620c12ad8f71414f6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 16:27:13 -0800 Subject: [PATCH 13/37] packer/rpc: MuxConn can return next available stream ID --- packer/rpc/muxconn.go | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index ee34b515f..7247d4ed1 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -18,8 +18,9 @@ import ( // are established using a subset of the TCP protocol. Only a subset is // necessary since we assume ordering on the underlying RWC. type MuxConn struct { + curId uint32 rwc io.ReadWriteCloser - streams map[byte]*Stream + streams map[uint32]*Stream mu sync.RWMutex wlock sync.Mutex } @@ -36,7 +37,7 @@ const ( func NewMuxConn(rwc io.ReadWriteCloser) *MuxConn { m := &MuxConn{ rwc: rwc, - streams: make(map[byte]*Stream), + streams: make(map[uint32]*Stream), } go m.loop() @@ -54,14 +55,14 @@ func (m *MuxConn) Close() error { for _, w := range m.streams { w.Close() } - m.streams = make(map[byte]*Stream) + m.streams = make(map[uint32]*Stream) return m.rwc.Close() } // Accept accepts a multiplexed connection with the given ID. This // will block until a request is made to connect. -func (m *MuxConn) Accept(id byte) (io.ReadWriteCloser, error) { +func (m *MuxConn) Accept(id uint32) (io.ReadWriteCloser, error) { stream, err := m.openStream(id) if err != nil { return nil, err @@ -113,7 +114,7 @@ func (m *MuxConn) Accept(id byte) (io.ReadWriteCloser, error) { // Dial opens a connection to the remote end using the given stream ID. // An Accept on the remote end will only work with if the IDs match. -func (m *MuxConn) Dial(id byte) (io.ReadWriteCloser, error) { +func (m *MuxConn) Dial(id uint32) (io.ReadWriteCloser, error) { stream, err := m.openStream(id) if err != nil { return nil, err @@ -149,7 +150,22 @@ func (m *MuxConn) Dial(id byte) (io.ReadWriteCloser, error) { } } -func (m *MuxConn) openStream(id byte) (*Stream, error) { +// NextId returns the next available stream ID that isn't currently +// taken. +func (m *MuxConn) NextId() uint32 { + m.mu.Lock() + defer m.mu.Unlock() + + for { + if _, ok := m.streams[m.curId]; !ok { + return m.curId + } + + m.curId++ + } +} + +func (m *MuxConn) openStream(id uint32) (*Stream, error) { m.mu.Lock() defer m.mu.Unlock() @@ -176,7 +192,7 @@ func (m *MuxConn) openStream(id byte) (*Stream, error) { func (m *MuxConn) loop() { defer m.Close() - var id byte + var id uint32 var packetType muxPacketType var length int32 for { @@ -249,7 +265,7 @@ func (m *MuxConn) loop() { } } -func (m *MuxConn) write(id byte, dataType muxPacketType, p []byte) (int, error) { +func (m *MuxConn) write(id uint32, dataType muxPacketType, p []byte) (int, error) { m.wlock.Lock() defer m.wlock.Unlock() @@ -270,7 +286,7 @@ func (m *MuxConn) write(id byte, dataType muxPacketType, p []byte) (int, error) // Stream is a single stream of data and implements io.ReadWriteCloser type Stream struct { - id byte + id uint32 mux *MuxConn reader io.Reader writer io.WriteCloser From 2ac629c94909f1b8efc982f9d2f54dbfcf504656 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2013 19:07:36 -0800 Subject: [PATCH 14/37] packer/rpc: get PostProcessor working --- packer/rpc/client.go | 17 ++++--- packer/rpc/muxconn.go | 2 +- packer/rpc/post_processor.go | 77 +++++++++++++------------------ packer/rpc/post_processor_test.go | 6 ++- packer/rpc/server_new.go | 19 +++++--- 5 files changed, 58 insertions(+), 63 deletions(-) diff --git a/packer/rpc/client.go b/packer/rpc/client.go index 99f0b296d..4dd9deb47 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -15,14 +15,12 @@ type Client struct { } func NewClient(rwc io.ReadWriteCloser) (*Client, error) { - // Create the MuxConn around the RWC and get the client to server stream. - // This is the primary stream that we use to communicate with the - // remote RPC server. On the remote side Server.ServeConn also listens - // on this stream ID. - mux := NewMuxConn(rwc) - clientConn, err := mux.Dial(0) + return NewClientWithMux(NewMuxConn(rwc), 0) +} + +func NewClientWithMux(mux *MuxConn, streamId uint32) (*Client, error) { + clientConn, err := mux.Dial(streamId) if err != nil { - mux.Close() return nil, err } @@ -37,7 +35,7 @@ func (c *Client) Close() error { return err } - return c.mux.Close() + return nil } func (c *Client) Artifact() packer.Artifact { @@ -56,12 +54,13 @@ func (c *Client) Cache() packer.Cache { func (c *Client) PostProcessor() packer.PostProcessor { return &postProcessor{ client: c.client, + mux: c.mux, } } func (c *Client) Ui() packer.Ui { return &Ui{ - client: c.client, + client: c.client, endpoint: DefaultUiEndpoint, } } diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 7247d4ed1..24e57fd63 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -21,7 +21,7 @@ type MuxConn struct { curId uint32 rwc io.ReadWriteCloser streams map[uint32]*Stream - mu sync.RWMutex + mu sync.Mutex wlock sync.Mutex } diff --git a/packer/rpc/post_processor.go b/packer/rpc/post_processor.go index 20e4f9b70..2a0dfe62a 100644 --- a/packer/rpc/post_processor.go +++ b/packer/rpc/post_processor.go @@ -9,14 +9,14 @@ import ( // executed over an RPC connection. type postProcessor struct { client *rpc.Client - server *rpc.Server + mux *MuxConn } // PostProcessorServer wraps a packer.PostProcessor implementation and makes it // exportable as part of a Golang RPC server. type PostProcessorServer struct { client *rpc.Client - server *rpc.Server + mux *MuxConn p packer.PostProcessor } @@ -24,15 +24,10 @@ type PostProcessorConfigureArgs struct { Configs []interface{} } -type PostProcessorPostProcessArgs struct { - ArtifactEndpoint string - UiEndpoint string -} - type PostProcessorProcessResponse struct { - Err error - Keep bool - ArtifactEndpoint string + Err error + Keep bool + StreamId uint32 } func PostProcessor(client *rpc.Client) *postProcessor { @@ -49,21 +44,14 @@ func (p *postProcessor) Configure(raw ...interface{}) (err error) { } func (p *postProcessor) PostProcess(ui packer.Ui, a packer.Artifact) (packer.Artifact, bool, error) { - artifactEndpoint := registerComponent(p.server, "Artifact", &ArtifactServer{ - artifact: a, - }, true) - - uiEndpoint := registerComponent(p.server, "Ui", &UiServer{ - ui: ui, - }, true) - - args := PostProcessorPostProcessArgs{ - ArtifactEndpoint: artifactEndpoint, - UiEndpoint: uiEndpoint, - } + nextId := p.mux.NextId() + server := NewServerWithMux(p.mux, nextId) + server.RegisterArtifact(a) + server.RegisterUi(ui) + go server.Serve() var response PostProcessorProcessResponse - if err := p.client.Call("PostProcessor.PostProcess", &args, &response); err != nil { + if err := p.client.Call("PostProcessor.PostProcess", nextId, &response); err != nil { return nil, false, err } @@ -71,14 +59,16 @@ func (p *postProcessor) PostProcess(ui packer.Ui, a packer.Artifact) (packer.Art return nil, false, response.Err } - if response.ArtifactEndpoint == "" { + if response.StreamId == 0 { return nil, false, nil } - return &artifact{ - client: p.client, - endpoint: response.ArtifactEndpoint, - }, response.Keep, nil + client, err := NewClientWithMux(p.mux, response.StreamId) + if err != nil { + return nil, false, err + } + + return client.Artifact(), response.Keep, nil } func (p *PostProcessorServer) Configure(args *PostProcessorConfigureArgs, reply *error) error { @@ -90,23 +80,20 @@ func (p *PostProcessorServer) Configure(args *PostProcessorConfigureArgs, reply return nil } -func (p *PostProcessorServer) PostProcess(args *PostProcessorPostProcessArgs, reply *PostProcessorProcessResponse) error { - artifact := &artifact{ - client: p.client, - endpoint: args.ArtifactEndpoint, +func (p *PostProcessorServer) PostProcess(streamId uint32, reply *PostProcessorProcessResponse) error { + client, err := NewClientWithMux(p.mux, streamId) + if err != nil { + return NewBasicError(err) } + defer client.Close() - ui := &Ui{ - client: p.client, - endpoint: args.UiEndpoint, - } - - var artifactEndpoint string - artifactResult, keep, err := p.p.PostProcess(ui, artifact) + streamId = 0 + artifactResult, keep, err := p.p.PostProcess(client.Ui(), client.Artifact()) if err == nil && artifactResult != nil { - artifactEndpoint = registerComponent(p.server, "Artifact", &ArtifactServer{ - artifact: artifactResult, - }, true) + streamId = p.mux.NextId() + server := NewServerWithMux(p.mux, streamId) + server.RegisterArtifact(artifactResult) + go server.Serve() } if err != nil { @@ -114,9 +101,9 @@ func (p *PostProcessorServer) PostProcess(args *PostProcessorPostProcessArgs, re } *reply = PostProcessorProcessResponse{ - Err: err, - Keep: keep, - ArtifactEndpoint: artifactEndpoint, + Err: err, + Keep: keep, + StreamId: streamId, } return nil diff --git a/packer/rpc/post_processor_test.go b/packer/rpc/post_processor_test.go index 418889ac5..d469ee4e0 100644 --- a/packer/rpc/post_processor_test.go +++ b/packer/rpc/post_processor_test.go @@ -13,6 +13,7 @@ type TestPostProcessor struct { configVal []interface{} ppCalled bool ppArtifact packer.Artifact + ppArtifactId string ppUi packer.Ui } @@ -25,6 +26,7 @@ func (pp *TestPostProcessor) Configure(v ...interface{}) error { func (pp *TestPostProcessor) PostProcess(ui packer.Ui, a packer.Artifact) (packer.Artifact, bool, error) { pp.ppCalled = true pp.ppArtifact = a + pp.ppArtifactId = a.Id() pp.ppUi = ui return testPostProcessorArtifact, false, nil } @@ -70,8 +72,8 @@ func TestPostProcessorRPC(t *testing.T) { t.Fatal("postprocess should be called") } - if p.ppArtifact.Id() != "ppTestId" { - t.Fatal("unknown artifact") + if p.ppArtifactId != "ppTestId" { + t.Fatalf("unknown artifact: %s", p.ppArtifact.Id()) } if artifact.Id() != "id" { diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index afed8fe58..4f531f187 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -21,15 +21,21 @@ const ( // Server represents an RPC server for Packer. This must be paired on // the other side with a Client. type Server struct { - mux *MuxConn - server *rpc.Server + mux *MuxConn + streamId uint32 + server *rpc.Server } // NewServer returns a new Packer RPC server. func NewServer(conn io.ReadWriteCloser) *Server { + return NewServerWithMux(NewMuxConn(conn), 0) +} + +func NewServerWithMux(mux *MuxConn, streamId uint32) *Server { return &Server{ - mux: NewMuxConn(conn), - server: rpc.NewServer(), + mux: mux, + streamId: streamId, + server: rpc.NewServer(), } } @@ -51,7 +57,8 @@ func (s *Server) RegisterCache(c packer.Cache) { func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { s.server.RegisterName(DefaultPostProcessorEndpoint, &PostProcessorServer{ - p: p, + mux: s.mux, + p: p, }) } @@ -66,7 +73,7 @@ func (s *Server) RegisterUi(ui packer.Ui) { func (s *Server) Serve() { // Accept a connection on stream ID 0, which is always used for // normal client to server connections. - stream, err := s.mux.Accept(0) + stream, err := s.mux.Accept(s.streamId) defer stream.Close() if err != nil { log.Printf("[ERR] Error retrieving stream for serving: %s", err) From 68e51de0f8be9081ac1b3e711625aa214041d77a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 10:34:35 -0800 Subject: [PATCH 15/37] packer/rpc: MuxConn.NextId properly increments --- packer/rpc/muxconn.go | 12 ++++++------ packer/rpc/muxconn_test.go | 13 +++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 24e57fd63..0e7c08ca0 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -72,7 +72,7 @@ func (m *MuxConn) Accept(id uint32) (io.ReadWriteCloser, error) { stream.mu.Lock() if stream.state != streamStateSynRecv && stream.state != streamStateClosed { stream.mu.Unlock() - return nil, fmt.Errorf("Stream already open in bad state: %d", stream.state) + return nil, fmt.Errorf("Stream %d already open in bad state: %d", id, stream.state) } if stream.state == streamStateSynRecv { @@ -124,7 +124,7 @@ func (m *MuxConn) Dial(id uint32) (io.ReadWriteCloser, error) { stream.mu.Lock() if stream.state != streamStateClosed { stream.mu.Unlock() - return nil, fmt.Errorf("Stream already open in bad state: %d", stream.state) + return nil, fmt.Errorf("Stream %d already open in bad state: %d", id, stream.state) } // Open a connection @@ -157,11 +157,11 @@ func (m *MuxConn) NextId() uint32 { defer m.mu.Unlock() for { - if _, ok := m.streams[m.curId]; !ok { - return m.curId - } - + result := m.curId m.curId++ + if _, ok := m.streams[result]; !ok { + return result + } } } diff --git a/packer/rpc/muxconn_test.go b/packer/rpc/muxconn_test.go index fce29b3af..e6c23f26f 100644 --- a/packer/rpc/muxconn_test.go +++ b/packer/rpc/muxconn_test.go @@ -159,3 +159,16 @@ func TestMuxConn_serverClosesStreams(t *testing.T) { t.Fatalf("err: %s", err) } } + +func TestMuxConnNextId(t *testing.T) { + client, server := testMux(t) + defer client.Close() + defer server.Close() + + a := client.NextId() + b := client.NextId() + + if a != 0 || b != 1 { + t.Fatalf("IDs should increment") + } +} From af22b35a1f08c512c194fc09f62d04dd0ea3db5e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 10:44:57 -0800 Subject: [PATCH 16/37] packer/rpc: MuxConn writes don't block the whole loop --- packer/rpc/muxconn.go | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 0e7c08ca0..13e864b5f 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -175,16 +175,29 @@ func (m *MuxConn) openStream(id uint32) (*Stream, error) { // Create the stream object and channel where data will be sent to dataR, dataW := io.Pipe() + writeCh := make(chan []byte, 10) // Set the data channel so we can write to it. stream := &Stream{ - id: id, - mux: m, - reader: dataR, - writer: dataW, + id: id, + mux: m, + reader: dataR, + writer: dataW, + writeCh: writeCh, } stream.setState(streamStateClosed) + // Start the goroutine that will read from the queue and write + // data out. + go func() { + for { + data := <-writeCh + if _, err := dataW.Write(data); err != nil { + return + } + } + }() + m.streams[id] = stream return m.streams[id], nil } @@ -256,7 +269,11 @@ func (m *MuxConn) loop() { case muxPacketData: stream.mu.Lock() if stream.state == streamStateEstablished { - stream.writer.Write(data) + select { + case stream.writeCh <- data: + default: + log.Printf("[ERR] Failed to write data, buffer full: %d", id) + } } else { log.Printf("[ERR] Data received for stream in state: %d", stream.state) } @@ -293,6 +310,7 @@ type Stream struct { state streamState stateUpdated time.Time mu sync.Mutex + writeCh chan<- []byte } type streamState byte From 72fcb566a6902471ee0abdd396b1d65ca1697ab5 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 11:40:17 -0800 Subject: [PATCH 17/37] packer/rpc: better close states --- packer/rpc/muxconn.go | 107 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 88 insertions(+), 19 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 13e864b5f..f5f0ce804 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -21,7 +21,7 @@ type MuxConn struct { curId uint32 rwc io.ReadWriteCloser streams map[uint32]*Stream - mu sync.Mutex + mu sync.RWMutex wlock sync.Mutex } @@ -48,8 +48,8 @@ func NewMuxConn(rwc io.ReadWriteCloser) *MuxConn { // Close closes the underlying io.ReadWriteCloser. This will also close // all streams that are open. func (m *MuxConn) Close() error { - m.mu.Lock() - defer m.mu.Unlock() + m.mu.RLock() + defer m.mu.RUnlock() // Close all the streams for _, w := range m.streams { @@ -94,12 +94,17 @@ func (m *MuxConn) Accept(id uint32) (io.ReadWriteCloser, error) { switch stream.state { case streamStateListen: stream.mu.Unlock() + case streamStateClosed: + // This can happen if it becomes established, some data is sent, + // and it closed all within the time period we wait above. + // This case will be fixed when we have edge-triggered checks. + fallthrough case streamStateEstablished: stream.mu.Unlock() break ACCEPT_ESTABLISH_LOOP default: defer stream.mu.Unlock() - return nil, fmt.Errorf("Stream went to bad state: %d", stream.state) + return nil, fmt.Errorf("Stream %d went to bad state: %d", id, stream.state) } } } @@ -140,12 +145,17 @@ func (m *MuxConn) Dial(id uint32) (io.ReadWriteCloser, error) { switch stream.state { case streamStateSynSent: stream.mu.Unlock() + case streamStateClosed: + // This can happen if it becomes established, some data is sent, + // and it closed all within the time period we wait above. + // This case will be fixed when we have edge-triggered checks. + fallthrough case streamStateEstablished: stream.mu.Unlock() return stream, nil default: defer stream.mu.Unlock() - return nil, fmt.Errorf("Stream went to bad state: %d", stream.state) + return nil, fmt.Errorf("Stream %d went to bad state: %d", id, stream.state) } } } @@ -166,9 +176,21 @@ func (m *MuxConn) NextId() uint32 { } func (m *MuxConn) openStream(id uint32) (*Stream, error) { + // First grab a read-lock if we have the stream already we can + // cheaply return it. + m.mu.RLock() + if stream, ok := m.streams[id]; ok { + m.mu.RUnlock() + return stream, nil + } + + // Now acquire a full blown write lock so we can create the stream + m.mu.RUnlock() m.mu.Lock() defer m.mu.Unlock() + // We have to check this again because there is a time period + // above where we couldn't lost this lock. if stream, ok := m.streams[id]; ok { return stream, nil } @@ -182,7 +204,6 @@ func (m *MuxConn) openStream(id uint32) (*Stream, error) { id: id, mux: m, reader: dataR, - writer: dataW, writeCh: writeCh, } stream.setState(streamStateClosed) @@ -190,8 +211,16 @@ func (m *MuxConn) openStream(id uint32) (*Stream, error) { // Start the goroutine that will read from the queue and write // data out. go func() { + defer dataW.Close() + for { data := <-writeCh + if data == nil { + // A nil is a tombstone letting us know we're done + // accepting data. + return + } + if _, err := dataW.Write(data); err != nil { return } @@ -237,12 +266,16 @@ func (m *MuxConn) loop() { return } + log.Printf("[DEBUG] Stream %d received packet %d", id, packetType) switch packetType { case muxPacketAck: stream.mu.Lock() - if stream.state == streamStateSynSent { + switch stream.state { + case streamStateSynSent: stream.setState(streamStateEstablished) - } else { + case streamStateFinWait1: + stream.remoteClose() + default: log.Printf("[ERR] Ack received for stream in state: %d", stream.state) } stream.mu.Unlock() @@ -259,13 +292,23 @@ func (m *MuxConn) loop() { stream.mu.Unlock() case muxPacketFin: stream.mu.Lock() - stream.setState(streamStateClosed) - stream.writer.Close() + switch stream.state { + case streamStateEstablished: + m.write(id, muxPacketAck, nil) + fallthrough + case streamStateFinWait1: + stream.remoteClose() + + // Remove this stream from being active so that it + // can be re-used + m.mu.Lock() + delete(m.streams, stream.id) + m.mu.Unlock() + default: + log.Printf("[ERR] Fin received for stream %d in state: %d", id, stream.state) + } stream.mu.Unlock() - m.mu.Lock() - delete(m.streams, stream.id) - m.mu.Unlock() case muxPacketData: stream.mu.Lock() if stream.state == streamStateEstablished { @@ -306,7 +349,6 @@ type Stream struct { id uint32 mux *MuxConn reader io.Reader - writer io.WriteCloser state streamState stateUpdated time.Time mu sync.Mutex @@ -321,23 +363,37 @@ const ( streamStateSynRecv streamStateSynSent streamStateEstablished - streamStateFinWait + streamStateFinWait1 ) func (s *Stream) Close() error { s.mu.Lock() - defer s.mu.Unlock() - if s.state != streamStateEstablished { + s.mu.Unlock() return fmt.Errorf("Stream in bad state: %d", s.state) } if _, err := s.mux.write(s.id, muxPacketFin, nil); err != nil { return err } + s.setState(streamStateFinWait1) + s.mu.Unlock() + + for { + time.Sleep(50 * time.Millisecond) + s.mu.Lock() + switch s.state { + case streamStateFinWait1: + s.mu.Unlock() + case streamStateClosed: + s.mu.Unlock() + return nil + default: + defer s.mu.Unlock() + return fmt.Errorf("Stream %d went to bad state: %d", s.id, s.state) + } + } - s.setState(streamStateClosed) - s.writer.Close() return nil } @@ -346,9 +402,22 @@ func (s *Stream) Read(p []byte) (int, error) { } func (s *Stream) Write(p []byte) (int, error) { + s.mu.Lock() + state := s.state + s.mu.Unlock() + + if state != streamStateEstablished { + return 0, fmt.Errorf("Stream in bad state to send: %d", state) + } + return s.mux.write(s.id, muxPacketData, p) } +func (s *Stream) remoteClose() { + s.setState(streamStateClosed) + s.writeCh <- nil +} + func (s *Stream) setState(state streamState) { s.state = state s.stateUpdated = time.Now().UTC() From db06fc7501198059206d3465829dfce611828485 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 11:43:02 -0800 Subject: [PATCH 18/37] packer/rpc: implement Communicator --- packer/rpc/client.go | 7 ++ packer/rpc/communicator.go | 133 +++++++++++++------------------- packer/rpc/communicator_test.go | 20 ++--- packer/rpc/muxconn.go | 2 +- packer/rpc/server.go | 2 +- packer/rpc/server_new.go | 8 ++ 6 files changed, 77 insertions(+), 95 deletions(-) diff --git a/packer/rpc/client.go b/packer/rpc/client.go index 4dd9deb47..cc657741e 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -51,6 +51,13 @@ func (c *Client) Cache() packer.Cache { } } +func (c *Client) Communicator() packer.Communicator { + return &communicator{ + client: c.client, + mux: c.mux, + } +} + func (c *Client) PostProcessor() packer.PostProcessor { return &postProcessor{ client: c.client, diff --git a/packer/rpc/communicator.go b/packer/rpc/communicator.go index 77e321153..de624a7af 100644 --- a/packer/rpc/communicator.go +++ b/packer/rpc/communicator.go @@ -2,11 +2,9 @@ package rpc import ( "encoding/gob" - "errors" "github.com/mitchellh/packer/packer" "io" "log" - "net" "net/rpc" ) @@ -14,12 +12,14 @@ import ( // executed over an RPC connection. type communicator struct { client *rpc.Client + mux *MuxConn } // CommunicatorServer wraps a packer.Communicator implementation and makes // it exportable as part of a Golang RPC server. type CommunicatorServer struct { - c packer.Communicator + c packer.Communicator + mux *MuxConn } type CommandFinished struct { @@ -27,21 +27,21 @@ type CommandFinished struct { } type CommunicatorStartArgs struct { - Command string - StdinAddress string - StdoutAddress string - StderrAddress string - ResponseAddress string + Command string + StdinStreamId uint32 + StdoutStreamId uint32 + StderrStreamId uint32 + ResponseStreamId uint32 } type CommunicatorDownloadArgs struct { - Path string - WriterAddress string + Path string + WriterStreamId uint32 } type CommunicatorUploadArgs struct { - Path string - ReaderAddress string + Path string + ReaderStreamId uint32 } type CommunicatorUploadDirArgs struct { @@ -51,7 +51,7 @@ type CommunicatorUploadDirArgs struct { } func Communicator(client *rpc.Client) *communicator { - return &communicator{client} + return &communicator{client: client} } func (c *communicator) Start(cmd *packer.RemoteCmd) (err error) { @@ -59,41 +59,38 @@ func (c *communicator) Start(cmd *packer.RemoteCmd) (err error) { args.Command = cmd.Command if cmd.Stdin != nil { - stdinL := netListenerInRange(portRangeMin, portRangeMax) - args.StdinAddress = stdinL.Addr().String() - go serveSingleCopy("stdin", stdinL, nil, cmd.Stdin) + args.StdinStreamId = c.mux.NextId() + go serveSingleCopy("stdin", c.mux, args.StdinStreamId, nil, cmd.Stdin) } if cmd.Stdout != nil { - stdoutL := netListenerInRange(portRangeMin, portRangeMax) - args.StdoutAddress = stdoutL.Addr().String() - go serveSingleCopy("stdout", stdoutL, cmd.Stdout, nil) + args.StdoutStreamId = c.mux.NextId() + go serveSingleCopy("stdout", c.mux, args.StdoutStreamId, cmd.Stdout, nil) } if cmd.Stderr != nil { - stderrL := netListenerInRange(portRangeMin, portRangeMax) - args.StderrAddress = stderrL.Addr().String() - go serveSingleCopy("stderr", stderrL, cmd.Stderr, nil) + args.StderrStreamId = c.mux.NextId() + go serveSingleCopy("stderr", c.mux, args.StderrStreamId, cmd.Stderr, nil) } - responseL := netListenerInRange(portRangeMin, portRangeMax) - args.ResponseAddress = responseL.Addr().String() + responseStreamId := c.mux.NextId() + args.ResponseStreamId = responseStreamId go func() { - defer responseL.Close() - - conn, err := responseL.Accept() + conn, err := c.mux.Accept(responseStreamId) if err != nil { + log.Printf("[ERR] Error accepting response stream %d: %s", + responseStreamId, err) cmd.SetExited(123) return } - defer conn.Close() - decoder := gob.NewDecoder(conn) - var finished CommandFinished + decoder := gob.NewDecoder(conn) if err := decoder.Decode(&finished); err != nil { + log.Printf("[ERR] Error decoding response stream %d: %s", + responseStreamId, err) cmd.SetExited(123) return } @@ -106,23 +103,13 @@ func (c *communicator) Start(cmd *packer.RemoteCmd) (err error) { } func (c *communicator) Upload(path string, r io.Reader) (err error) { - // We need to create a server that can proxy the reader data - // over because we can't simply gob encode an io.Reader - readerL := netListenerInRange(portRangeMin, portRangeMax) - if readerL == nil { - err = errors.New("couldn't allocate listener for upload reader") - return - } - - // Make sure at the end of this call, we close the listener - defer readerL.Close() - // Pipe the reader through to the connection - go serveSingleCopy("uploadReader", readerL, nil, r) + streamId := c.mux.NextId() + go serveSingleCopy("uploadReader", c.mux, streamId, nil, r) args := CommunicatorUploadArgs{ - path, - readerL.Addr().String(), + Path: path, + ReaderStreamId: streamId, } err = c.client.Call("Communicator.Upload", &args, new(interface{})) @@ -146,23 +133,13 @@ func (c *communicator) UploadDir(dst string, src string, exclude []string) error } func (c *communicator) Download(path string, w io.Writer) (err error) { - // We need to create a server that can proxy that data downloaded - // into the writer because we can't gob encode a writer directly. - writerL := netListenerInRange(portRangeMin, portRangeMax) - if writerL == nil { - err = errors.New("couldn't allocate listener for download writer") - return - } - - // Make sure we close the listener once we're done because we'll be done - defer writerL.Close() - // Serve a single connection and a single copy - go serveSingleCopy("downloadWriter", writerL, w, nil) + streamId := c.mux.NextId() + go serveSingleCopy("downloadWriter", c.mux, streamId, w, nil) args := CommunicatorDownloadArgs{ - path, - writerL.Addr().String(), + Path: path, + WriterStreamId: streamId, } err = c.client.Call("Communicator.Download", &args, new(interface{})) @@ -175,40 +152,40 @@ func (c *CommunicatorServer) Start(args *CommunicatorStartArgs, reply *interface var cmd packer.RemoteCmd cmd.Command = args.Command - toClose := make([]net.Conn, 0) - if args.StdinAddress != "" { - stdinC, err := tcpDial(args.StdinAddress) + toClose := make([]io.Closer, 0) + if args.StdinStreamId > 0 { + conn, err := c.mux.Dial(args.StdinStreamId) if err != nil { return err } - toClose = append(toClose, stdinC) - cmd.Stdin = stdinC + toClose = append(toClose, conn) + cmd.Stdin = conn } - if args.StdoutAddress != "" { - stdoutC, err := tcpDial(args.StdoutAddress) + if args.StdoutStreamId > 0 { + conn, err := c.mux.Dial(args.StdoutStreamId) if err != nil { return err } - toClose = append(toClose, stdoutC) - cmd.Stdout = stdoutC + toClose = append(toClose, conn) + cmd.Stdout = conn } - if args.StderrAddress != "" { - stderrC, err := tcpDial(args.StderrAddress) + if args.StderrStreamId > 0 { + conn, err := c.mux.Dial(args.StderrStreamId) if err != nil { return err } - toClose = append(toClose, stderrC) - cmd.Stderr = stderrC + toClose = append(toClose, conn) + cmd.Stderr = conn } // Connect to the response address so we can write our result to it // when ready. - responseC, err := tcpDial(args.ResponseAddress) + responseC, err := c.mux.Dial(args.ResponseStreamId) if err != nil { return err } @@ -234,11 +211,10 @@ func (c *CommunicatorServer) Start(args *CommunicatorStartArgs, reply *interface } func (c *CommunicatorServer) Upload(args *CommunicatorUploadArgs, reply *interface{}) (err error) { - readerC, err := tcpDial(args.ReaderAddress) + readerC, err := c.mux.Dial(args.ReaderStreamId) if err != nil { return } - defer readerC.Close() err = c.c.Upload(args.Path, readerC) @@ -250,21 +226,18 @@ func (c *CommunicatorServer) UploadDir(args *CommunicatorUploadDirArgs, reply *e } func (c *CommunicatorServer) Download(args *CommunicatorDownloadArgs, reply *interface{}) (err error) { - writerC, err := tcpDial(args.WriterAddress) + writerC, err := c.mux.Dial(args.WriterStreamId) if err != nil { return } - defer writerC.Close() err = c.c.Download(args.Path, writerC) return } -func serveSingleCopy(name string, l net.Listener, dst io.Writer, src io.Reader) { - defer l.Close() - - conn, err := l.Accept() +func serveSingleCopy(name string, mux *MuxConn, id uint32, dst io.Writer, src io.Reader) { + conn, err := mux.Accept(id) if err != nil { log.Printf("'%s' accept error: %s", name, err) return diff --git a/packer/rpc/communicator_test.go b/packer/rpc/communicator_test.go index f6013a061..ca8239514 100644 --- a/packer/rpc/communicator_test.go +++ b/packer/rpc/communicator_test.go @@ -4,7 +4,6 @@ import ( "bufio" "github.com/mitchellh/packer/packer" "io" - "net/rpc" "reflect" "testing" ) @@ -14,16 +13,11 @@ func TestCommunicatorRPC(t *testing.T) { c := new(packer.MockCommunicator) // Start the server - server := rpc.NewServer() - RegisterCommunicator(server, c) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - remote := Communicator(client) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterCommunicator(c) + remote := client.Communicator() // The remote command we'll use stdin_r, stdin_w := io.Pipe() @@ -42,7 +36,7 @@ func TestCommunicatorRPC(t *testing.T) { c.StartExitStatus = 42 // Test Start - err = remote.Start(&cmd) + err := remote.Start(&cmd) if err != nil { t.Fatalf("err: %s", err) } @@ -74,7 +68,7 @@ func TestCommunicatorRPC(t *testing.T) { stdin_w.Close() cmd.Wait() if c.StartStdin != "info\n" { - t.Fatalf("bad data: %s", data) + t.Fatalf("bad data: %s", c.StartStdin) } // Test that we can get the exit status properly diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index f5f0ce804..87941b826 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -266,7 +266,7 @@ func (m *MuxConn) loop() { return } - log.Printf("[DEBUG] Stream %d received packet %d", id, packetType) + //log.Printf("[DEBUG] Stream %d received packet %d", id, packetType) switch packetType { case muxPacketAck: stream.mu.Lock() diff --git a/packer/rpc/server.go b/packer/rpc/server.go index 68116885b..0d6cacf62 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -38,7 +38,7 @@ func RegisterCommand(s *rpc.Server, c packer.Command) { // Registers the appropriate endpoint on an RPC server to serve a // Packer Communicator. func RegisterCommunicator(s *rpc.Server, c packer.Communicator) { - registerComponent(s, "Communicator", &CommunicatorServer{c}, false) + registerComponent(s, "Communicator", &CommunicatorServer{c: c}, false) } // Registers the appropriate endpoint on an RPC server to serve a diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 4f531f187..8a1d0a128 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -14,6 +14,7 @@ var endpointId uint64 const ( DefaultArtifactEndpoint string = "Artifact" DefaultCacheEndpoint = "Cache" + DefaultCommunicatorEndpoint = "Communicator" DefaultPostProcessorEndpoint = "PostProcessor" DefaultUiEndpoint = "Ui" ) @@ -55,6 +56,13 @@ func (s *Server) RegisterCache(c packer.Cache) { }) } +func (s *Server) RegisterCommunicator(c packer.Communicator) { + s.server.RegisterName(DefaultCommunicatorEndpoint, &CommunicatorServer{ + c: c, + mux: s.mux, + }) +} + func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { s.server.RegisterName(DefaultPostProcessorEndpoint, &PostProcessorServer{ mux: s.mux, From a036bec96e69f3ebe21f13251a7b57453f006161 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 11:50:30 -0800 Subject: [PATCH 19/37] packer/rpc: Hook --- packer/rpc/client.go | 7 +++++++ packer/rpc/hook.go | 35 ++++++++++++++++++++++------------- packer/rpc/hook_test.go | 33 ++++++++++----------------------- packer/rpc/server.go | 2 +- packer/rpc/server_new.go | 8 ++++++++ 5 files changed, 48 insertions(+), 37 deletions(-) diff --git a/packer/rpc/client.go b/packer/rpc/client.go index cc657741e..c201ebe19 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -58,6 +58,13 @@ func (c *Client) Communicator() packer.Communicator { } } +func (c *Client) Hook() packer.Hook { + return &hook{ + client: c.client, + mux: c.mux, + } +} + func (c *Client) PostProcessor() packer.PostProcessor { return &postProcessor{ client: c.client, diff --git a/packer/rpc/hook.go b/packer/rpc/hook.go index cede08302..8162dd4f2 100644 --- a/packer/rpc/hook.go +++ b/packer/rpc/hook.go @@ -10,32 +10,40 @@ import ( // over an RPC connection. type hook struct { client *rpc.Client + mux *MuxConn } // HookServer wraps a packer.Hook implementation and makes it exportable // as part of a Golang RPC server. type HookServer struct { hook packer.Hook + mux *MuxConn } type HookRunArgs struct { - Name string - Data interface{} - RPCAddress string + Name string + Data interface{} + StreamId uint32 } func Hook(client *rpc.Client) *hook { - return &hook{client} + return &hook{client: client} } func (h *hook) Run(name string, ui packer.Ui, comm packer.Communicator, data interface{}) error { - server := rpc.NewServer() - RegisterCommunicator(server, comm) - RegisterUi(server, ui) - address := serveSingleConn(server) + nextId := h.mux.NextId() + server := NewServerWithMux(h.mux, nextId) + server.RegisterCommunicator(comm) + server.RegisterUi(ui) + go server.Serve() - args := &HookRunArgs{name, data, address} - return h.client.Call("Hook.Run", args, new(interface{})) + args := HookRunArgs{ + Name: name, + Data: data, + StreamId: nextId, + } + + return h.client.Call("Hook.Run", &args, new(interface{})) } func (h *hook) Cancel() { @@ -46,12 +54,13 @@ func (h *hook) Cancel() { } func (h *HookServer) Run(args *HookRunArgs, reply *interface{}) error { - client, err := rpcDial(args.RPCAddress) + client, err := NewClientWithMux(h.mux, args.StreamId) if err != nil { - return err + return NewBasicError(err) } + defer client.Close() - if err := h.hook.Run(args.Name, &Ui{client: client}, Communicator(client), args.Data); err != nil { + if err := h.hook.Run(args.Name, client.Ui(), client.Communicator(), args.Data); err != nil { return NewBasicError(err) } diff --git a/packer/rpc/hook_test.go b/packer/rpc/hook_test.go index c7ffc7258..b3f4a420c 100644 --- a/packer/rpc/hook_test.go +++ b/packer/rpc/hook_test.go @@ -2,7 +2,6 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "sync" "testing" @@ -14,17 +13,11 @@ func TestHookRPC(t *testing.T) { h := new(packer.MockHook) // Serve - server := rpc.NewServer() - RegisterHook(server, h) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - - hClient := Hook(client) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterHook(h) + hClient := client.Hook() // Test Run ui := &testUi{} @@ -60,17 +53,11 @@ func TestHook_cancelWhileRun(t *testing.T) { } // Serve - server := rpc.NewServer() - RegisterHook(server, h) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - - hClient := Hook(client) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterHook(h) + hClient := client.Hook() // Start the run finished := make(chan struct{}) diff --git a/packer/rpc/server.go b/packer/rpc/server.go index 0d6cacf62..fc29c7e91 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -50,7 +50,7 @@ func RegisterEnvironment(s *rpc.Server, e packer.Environment) { // Registers the appropriate endpoint on an RPC server to serve a // Hook. func RegisterHook(s *rpc.Server, h packer.Hook) { - registerComponent(s, "Hook", &HookServer{h}, false) + registerComponent(s, "Hook", &HookServer{hook: h}, false) } // Registers the appropriate endpoing on an RPC server to serve a diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 8a1d0a128..b8cb9fffa 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -15,6 +15,7 @@ const ( DefaultArtifactEndpoint string = "Artifact" DefaultCacheEndpoint = "Cache" DefaultCommunicatorEndpoint = "Communicator" + DefaultHookEndpoint = "Hook" DefaultPostProcessorEndpoint = "PostProcessor" DefaultUiEndpoint = "Ui" ) @@ -63,6 +64,13 @@ func (s *Server) RegisterCommunicator(c packer.Communicator) { }) } +func (s *Server) RegisterHook(h packer.Hook) { + s.server.RegisterName(DefaultHookEndpoint, &HookServer{ + hook: h, + mux: s.mux, + }) +} + func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { s.server.RegisterName(DefaultPostProcessorEndpoint, &PostProcessorServer{ mux: s.mux, From 5966a6e905104c80c005c08bdda54e0ab92af2b7 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 11:56:15 -0800 Subject: [PATCH 20/37] packer/rpc: Provisioner --- packer/rpc/client.go | 7 +++++++ packer/rpc/provisioner.go | 34 +++++++++++++++------------------- packer/rpc/provisioner_test.go | 21 +++++---------------- packer/rpc/server.go | 2 +- packer/rpc/server_new.go | 8 ++++++++ 5 files changed, 36 insertions(+), 36 deletions(-) diff --git a/packer/rpc/client.go b/packer/rpc/client.go index c201ebe19..83783d79b 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -72,6 +72,13 @@ func (c *Client) PostProcessor() packer.PostProcessor { } } +func (c *Client) Provisioner() packer.Provisioner { + return &provisioner{ + client: c.client, + mux: c.mux, + } +} + func (c *Client) Ui() packer.Ui { return &Ui{ client: c.client, diff --git a/packer/rpc/provisioner.go b/packer/rpc/provisioner.go index a62f97d45..eafd65756 100644 --- a/packer/rpc/provisioner.go +++ b/packer/rpc/provisioner.go @@ -10,24 +10,22 @@ import ( // executed over an RPC connection. type provisioner struct { client *rpc.Client + mux *MuxConn } // ProvisionerServer wraps a packer.Provisioner implementation and makes it // exportable as part of a Golang RPC server. type ProvisionerServer struct { - p packer.Provisioner + p packer.Provisioner + mux *MuxConn } type ProvisionerPrepareArgs struct { Configs []interface{} } -type ProvisionerProvisionArgs struct { - RPCAddress string -} - func Provisioner(client *rpc.Client) *provisioner { - return &provisioner{client} + return &provisioner{client: client} } func (p *provisioner) Prepare(configs ...interface{}) (err error) { args := &ProvisionerPrepareArgs{configs} @@ -39,13 +37,13 @@ func (p *provisioner) Prepare(configs ...interface{}) (err error) { } func (p *provisioner) Provision(ui packer.Ui, comm packer.Communicator) error { - // TODO: Error handling - server := rpc.NewServer() - RegisterCommunicator(server, comm) - RegisterUi(server, ui) + nextId := p.mux.NextId() + server := NewServerWithMux(p.mux, nextId) + server.RegisterCommunicator(comm) + server.RegisterUi(ui) + go server.Serve() - args := &ProvisionerProvisionArgs{serveSingleConn(server)} - return p.client.Call("Provisioner.Provision", args, new(interface{})) + return p.client.Call("Provisioner.Provision", nextId, new(interface{})) } func (p *provisioner) Cancel() { @@ -64,16 +62,14 @@ func (p *ProvisionerServer) Prepare(args *ProvisionerPrepareArgs, reply *error) return nil } -func (p *ProvisionerServer) Provision(args *ProvisionerProvisionArgs, reply *interface{}) error { - client, err := rpcDial(args.RPCAddress) +func (p *ProvisionerServer) Provision(streamId uint32, reply *interface{}) error { + client, err := NewClientWithMux(p.mux, streamId) if err != nil { - return err + return NewBasicError(err) } + defer client.Close() - comm := Communicator(client) - ui := &Ui{client: client} - - if err := p.p.Provision(ui, comm); err != nil { + if err := p.p.Provision(client.Ui(), client.Communicator()); err != nil { return NewBasicError(err) } diff --git a/packer/rpc/provisioner_test.go b/packer/rpc/provisioner_test.go index 7e281a1b5..5d1604b74 100644 --- a/packer/rpc/provisioner_test.go +++ b/packer/rpc/provisioner_test.go @@ -2,7 +2,6 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "testing" ) @@ -12,19 +11,14 @@ func TestProvisionerRPC(t *testing.T) { p := new(packer.MockProvisioner) // Start the server - server := rpc.NewServer() - RegisterProvisioner(server, p) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterProvisioner(p) + pClient := client.Provisioner() // Test Prepare config := 42 - pClient := Provisioner(client) pClient.Prepare(config) if !p.PrepCalled { t.Fatal("should be called") @@ -41,11 +35,6 @@ func TestProvisionerRPC(t *testing.T) { t.Fatal("should be called") } - p.ProvUi.Say("foo") - if !ui.sayCalled { - t.Fatal("should be called") - } - // Test Cancel pClient.Cancel() if !p.CancelCalled { diff --git a/packer/rpc/server.go b/packer/rpc/server.go index fc29c7e91..904a1b1aa 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -61,7 +61,7 @@ func RegisterPostProcessor(s *rpc.Server, p packer.PostProcessor) { // Registers the appropriate endpoint on an RPC server to serve a packer.Provisioner func RegisterProvisioner(s *rpc.Server, p packer.Provisioner) { - registerComponent(s, "Provisioner", &ProvisionerServer{p}, false) + registerComponent(s, "Provisioner", &ProvisionerServer{p: p}, false) } // Registers the appropriate endpoint on an RPC server to serve a diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index b8cb9fffa..b80f6e441 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -17,6 +17,7 @@ const ( DefaultCommunicatorEndpoint = "Communicator" DefaultHookEndpoint = "Hook" DefaultPostProcessorEndpoint = "PostProcessor" + DefaultProvisionerEndpoint = "Provisioner" DefaultUiEndpoint = "Ui" ) @@ -78,6 +79,13 @@ func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { }) } +func (s *Server) RegisterProvisioner(p packer.Provisioner) { + s.server.RegisterName(DefaultProvisionerEndpoint, &ProvisionerServer{ + mux: s.mux, + p: p, + }) +} + func (s *Server) RegisterUi(ui packer.Ui) { s.server.RegisterName(DefaultUiEndpoint, &UiServer{ ui: ui, From e69399380ecdc423657b30bc235012335e201450 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 12:02:01 -0800 Subject: [PATCH 21/37] packer/rpc: start command --- packer/rpc/client.go | 7 +++++++ packer/rpc/command.go | 4 +++- packer/rpc/command_test.go | 24 ++++++++---------------- packer/rpc/server.go | 2 +- packer/rpc/server_new.go | 8 ++++++++ 5 files changed, 27 insertions(+), 18 deletions(-) diff --git a/packer/rpc/client.go b/packer/rpc/client.go index 83783d79b..e0729c009 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -51,6 +51,13 @@ func (c *Client) Cache() packer.Cache { } } +func (c *Client) Command() packer.Command { + return &command{ + client: c.client, + mux: c.mux, + } +} + func (c *Client) Communicator() packer.Communicator { return &communicator{ client: c.client, diff --git a/packer/rpc/command.go b/packer/rpc/command.go index 3e2b48b2f..4d45a2ec1 100644 --- a/packer/rpc/command.go +++ b/packer/rpc/command.go @@ -9,12 +9,14 @@ import ( // command is actually executed over an RPC connection. type command struct { client *rpc.Client + mux *MuxConn } // A CommandServer wraps a packer.Command and makes it exportable as part // of a Golang RPC server. type CommandServer struct { command packer.Command + mux *MuxConn } type CommandRunArgs struct { @@ -25,7 +27,7 @@ type CommandRunArgs struct { type CommandSynopsisArgs byte func Command(client *rpc.Client) *command { - return &command{client} + return &command{client: client} } func (c *command) Help() (result string) { diff --git a/packer/rpc/command_test.go b/packer/rpc/command_test.go index 086e0eeed..976b70e6f 100644 --- a/packer/rpc/command_test.go +++ b/packer/rpc/command_test.go @@ -2,7 +2,6 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "testing" ) @@ -33,21 +32,14 @@ func TestRPCCommand(t *testing.T) { command := new(TestCommand) // Start the server - server := rpc.NewServer() - RegisterCommand(server, command) - address := serveSingleConn(server) - - // Create the command client over RPC and run some methods to verify - // we get the proper behavior. - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - - clientComm := Command(client) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterCommand(command) + commClient := client.Command() //Test Help - help := clientComm.Help() + help := commClient.Help() if help != "bar" { t.Fatalf("bad: %s", help) } @@ -55,7 +47,7 @@ func TestRPCCommand(t *testing.T) { // Test run runArgs := []string{"foo", "bar"} testEnv := &testEnvironment{} - exitCode := clientComm.Run(testEnv, runArgs) + exitCode := commClient.Run(testEnv, runArgs) if !reflect.DeepEqual(command.runArgs, runArgs) { t.Fatalf("bad: %#v", command.runArgs) } @@ -73,7 +65,7 @@ func TestRPCCommand(t *testing.T) { } // Test Synopsis - synopsis := clientComm.Synopsis() + synopsis := commClient.Synopsis() if synopsis != "foo" { t.Fatalf("bad: %#v", synopsis) } diff --git a/packer/rpc/server.go b/packer/rpc/server.go index 904a1b1aa..177479e44 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -32,7 +32,7 @@ func RegisterCache(s *rpc.Server, c packer.Cache) { // Registers the appropriate endpoint on an RPC server to serve a // Packer Command. func RegisterCommand(s *rpc.Server, c packer.Command) { - registerComponent(s, "Command", &CommandServer{c}, false) + registerComponent(s, "Command", &CommandServer{command: c}, false) } // Registers the appropriate endpoint on an RPC server to serve a diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index b80f6e441..83ece27ee 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -14,6 +14,7 @@ var endpointId uint64 const ( DefaultArtifactEndpoint string = "Artifact" DefaultCacheEndpoint = "Cache" + DefaultCommandEndpoint = "Command" DefaultCommunicatorEndpoint = "Communicator" DefaultHookEndpoint = "Hook" DefaultPostProcessorEndpoint = "PostProcessor" @@ -58,6 +59,13 @@ func (s *Server) RegisterCache(c packer.Cache) { }) } +func (s *Server) RegisterCommand(c packer.Command) { + s.server.RegisterName(DefaultCommandEndpoint, &CommandServer{ + command: c, + mux: s.mux, + }) +} + func (s *Server) RegisterCommunicator(c packer.Communicator) { s.server.RegisterName(DefaultCommunicatorEndpoint, &CommunicatorServer{ c: c, From 2ba713d705e795ef21e392a6b76a3516fb17582b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 12:14:08 -0800 Subject: [PATCH 22/37] packer/rpc: Builder --- packer/rpc/builder.go | 119 ++++++++++--------------------------- packer/rpc/builder_test.go | 80 ++++++++++++------------- packer/rpc/client.go | 7 +++ packer/rpc/server.go | 2 +- packer/rpc/server_new.go | 8 +++ 5 files changed, 84 insertions(+), 132 deletions(-) diff --git a/packer/rpc/builder.go b/packer/rpc/builder.go index 0c5dfab37..20c166a58 100644 --- a/packer/rpc/builder.go +++ b/packer/rpc/builder.go @@ -1,8 +1,6 @@ package rpc import ( - "encoding/gob" - "fmt" "github.com/mitchellh/packer/packer" "log" "net/rpc" @@ -12,35 +10,27 @@ import ( // over an RPC connection. type builder struct { client *rpc.Client + mux *MuxConn } // BuilderServer wraps a packer.Builder implementation and makes it exportable // as part of a Golang RPC server. type BuilderServer struct { builder packer.Builder + mux *MuxConn } type BuilderPrepareArgs struct { Configs []interface{} } -type BuilderRunArgs struct { - RPCAddress string - ResponseAddress string -} - type BuilderPrepareResponse struct { Warnings []string Error error } -type BuilderRunResponse struct { - Err error - RPCAddress string -} - func Builder(client *rpc.Client) *builder { - return &builder{client} + return &builder{client: client} } func (b *builder) Prepare(config ...interface{}) ([]string, error) { @@ -54,58 +44,28 @@ func (b *builder) Prepare(config ...interface{}) ([]string, error) { } func (b *builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packer.Artifact, error) { - // Create and start the server for the Build and UI - server := rpc.NewServer() - RegisterCache(server, cache) - RegisterHook(server, hook) - RegisterUi(server, ui) + nextId := b.mux.NextId() + server := NewServerWithMux(b.mux, nextId) + server.RegisterCache(cache) + server.RegisterHook(hook) + server.RegisterUi(ui) + go server.Serve() - // Create a server for the response - responseL := netListenerInRange(portRangeMin, portRangeMax) - runResponseCh := make(chan *BuilderRunResponse) - go func() { - defer responseL.Close() - - var response BuilderRunResponse - defer func() { runResponseCh <- &response }() - - conn, err := responseL.Accept() - if err != nil { - response.Err = err - return - } - defer conn.Close() - - decoder := gob.NewDecoder(conn) - if err := decoder.Decode(&response); err != nil { - response.Err = fmt.Errorf("Error waiting for Run: %s", err) - } - }() - - args := &BuilderRunArgs{ - serveSingleConn(server), - responseL.Addr().String(), - } - - if err := b.client.Call("Builder.Run", args, new(interface{})); err != nil { + var responseId uint32 + if err := b.client.Call("Builder.Run", nextId, &responseId); err != nil { return nil, err } - response := <-runResponseCh - if response.Err != nil { - return nil, response.Err - } - - if response.RPCAddress == "" { + if responseId == 0 { return nil, nil } - client, err := rpcDial(response.RPCAddress) + client, err := NewClientWithMux(b.mux, responseId) if err != nil { return nil, err } - return Artifact(client), nil + return client.Artifact(), nil } func (b *builder) Cancel() { @@ -127,46 +87,27 @@ func (b *BuilderServer) Prepare(args *BuilderPrepareArgs, reply *BuilderPrepareR return nil } -func (b *BuilderServer) Run(args *BuilderRunArgs, reply *interface{}) error { - client, err := rpcDial(args.RPCAddress) +func (b *BuilderServer) Run(streamId uint32, reply *uint32) error { + client, err := NewClientWithMux(b.mux, streamId) if err != nil { - return err + return NewBasicError(err) + } + defer client.Close() + + artifact, err := b.builder.Run(client.Ui(), client.Hook(), client.Cache()) + if err != nil { + return NewBasicError(err) } - responseC, err := tcpDial(args.ResponseAddress) - if err != nil { - return err + *reply = 0 + if artifact != nil { + streamId = b.mux.NextId() + server := NewServerWithMux(b.mux, streamId) + server.RegisterArtifact(artifact) + go server.Serve() + *reply = streamId } - responseWriter := gob.NewEncoder(responseC) - - // Run the build in a goroutine so we don't block the RPC connection - go func() { - defer responseC.Close() - - cache := Cache(client) - hook := Hook(client) - ui := &Ui{client: client} - artifact, responseErr := b.builder.Run(ui, hook, cache) - responseAddress := "" - - if responseErr == nil && artifact != nil { - // Wrap the artifact - server := rpc.NewServer() - RegisterArtifact(server, artifact) - responseAddress = serveSingleConn(server) - } - - if responseErr != nil { - responseErr = NewBasicError(responseErr) - } - - err := responseWriter.Encode(&BuilderRunResponse{responseErr, responseAddress}) - if err != nil { - log.Printf("BuildServer.Run error: %s", err) - } - }() - return nil } diff --git a/packer/rpc/builder_test.go b/packer/rpc/builder_test.go index 81e65a0d1..22bbeea06 100644 --- a/packer/rpc/builder_test.go +++ b/packer/rpc/builder_test.go @@ -2,31 +2,19 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "testing" ) var testBuilderArtifact = &packer.MockArtifact{} -func builderRPCClient(t *testing.T) (*packer.MockBuilder, packer.Builder) { - b := new(packer.MockBuilder) - - // Start the server - server := rpc.NewServer() - RegisterBuilder(server, b) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - return b, Builder(client) -} - func TestBuilderPrepare(t *testing.T) { - b, bClient := builderRPCClient(t) + b := new(packer.MockBuilder) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuilder(b) + bClient := client.Builder() // Test Prepare config := 42 @@ -48,7 +36,12 @@ func TestBuilderPrepare(t *testing.T) { } func TestBuilderPrepare_Warnings(t *testing.T) { - b, bClient := builderRPCClient(t) + b := new(packer.MockBuilder) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuilder(b) + bClient := client.Builder() expected := []string{"foo"} b.PrepareWarnings = expected @@ -64,7 +57,12 @@ func TestBuilderPrepare_Warnings(t *testing.T) { } func TestBuilderRun(t *testing.T) { - b, bClient := builderRPCClient(t) + b := new(packer.MockBuilder) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuilder(b) + bClient := client.Builder() // Test Run cache := new(testCache) @@ -79,34 +77,21 @@ func TestBuilderRun(t *testing.T) { t.Fatal("run should be called") } - b.RunCache.Lock("foo") - if !cache.lockCalled { - t.Fatal("should be called") - } - - b.RunHook.Run("foo", nil, nil, nil) - if !hook.RunCalled { - t.Fatal("should be called") - } - - b.RunUi.Say("format") - if !ui.sayCalled { - t.Fatal("say should be called") - } - - if ui.sayMessage != "format" { - t.Fatalf("bad: %s", ui.sayMessage) - } - if artifact.Id() != testBuilderArtifact.Id() { t.Fatalf("bad: %s", artifact.Id()) } } func TestBuilderRun_nilResult(t *testing.T) { - b, bClient := builderRPCClient(t) + b := new(packer.MockBuilder) b.RunNilResult = true + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuilder(b) + bClient := client.Builder() + cache := new(testCache) hook := &packer.MockHook{} ui := &testUi{} @@ -120,7 +105,13 @@ func TestBuilderRun_nilResult(t *testing.T) { } func TestBuilderRun_ErrResult(t *testing.T) { - b, bClient := builderRPCClient(t) + b := new(packer.MockBuilder) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuilder(b) + bClient := client.Builder() + b.RunErrResult = true cache := new(testCache) @@ -136,7 +127,12 @@ func TestBuilderRun_ErrResult(t *testing.T) { } func TestBuilderCancel(t *testing.T) { - b, bClient := builderRPCClient(t) + b := new(packer.MockBuilder) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuilder(b) + bClient := client.Builder() bClient.Cancel() if !b.CancelCalled { diff --git a/packer/rpc/client.go b/packer/rpc/client.go index e0729c009..d82a20c7a 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -45,6 +45,13 @@ func (c *Client) Artifact() packer.Artifact { } } +func (c *Client) Builder() packer.Builder { + return &builder{ + client: c.client, + mux: c.mux, + } +} + func (c *Client) Cache() packer.Cache { return &cache{ client: c.client, diff --git a/packer/rpc/server.go b/packer/rpc/server.go index 177479e44..32190a2fe 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -20,7 +20,7 @@ func RegisterBuild(s *rpc.Server, b packer.Build) { // Registers the appropriate endpoint on an RPC server to serve a // Packer Builder. func RegisterBuilder(s *rpc.Server, b packer.Builder) { - registerComponent(s, "Builder", &BuilderServer{b}, false) + registerComponent(s, "Builder", &BuilderServer{builder: b}, false) } // Registers the appropriate endpoint on an RPC server to serve a diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 83ece27ee..53e4d0ddd 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -13,6 +13,7 @@ var endpointId uint64 const ( DefaultArtifactEndpoint string = "Artifact" + DefaultBuilderEndpoint = "Builder" DefaultCacheEndpoint = "Cache" DefaultCommandEndpoint = "Command" DefaultCommunicatorEndpoint = "Communicator" @@ -53,6 +54,13 @@ func (s *Server) RegisterArtifact(a packer.Artifact) { }) } +func (s *Server) RegisterBuilder(b packer.Builder) { + s.server.RegisterName(DefaultBuilderEndpoint, &BuilderServer{ + builder: b, + mux: s.mux, + }) +} + func (s *Server) RegisterCache(c packer.Cache) { s.server.RegisterName(DefaultCacheEndpoint, &CacheServer{ cache: c, From bd6fbc05ebd7d60f94f855589a8d52c78d95a58c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 12:23:42 -0800 Subject: [PATCH 23/37] packer/rpc: environment --- packer/rpc/client.go | 7 ++ packer/rpc/command.go | 2 +- packer/rpc/environment.go | 150 ++++++++++++++++----------------- packer/rpc/environment_test.go | 16 ++-- packer/rpc/server.go | 2 +- packer/rpc/server_new.go | 8 ++ 6 files changed, 95 insertions(+), 90 deletions(-) diff --git a/packer/rpc/client.go b/packer/rpc/client.go index d82a20c7a..237cb0c88 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -72,6 +72,13 @@ func (c *Client) Communicator() packer.Communicator { } } +func (c *Client) Environment() packer.Environment { + return &Environment{ + client: c.client, + mux: c.mux, + } +} + func (c *Client) Hook() packer.Hook { return &hook{ client: c.client, diff --git a/packer/rpc/command.go b/packer/rpc/command.go index 4d45a2ec1..945b8d58b 100644 --- a/packer/rpc/command.go +++ b/packer/rpc/command.go @@ -73,7 +73,7 @@ func (c *CommandServer) Run(args *CommandRunArgs, reply *int) error { return err } - env := &Environment{client} + env := &Environment{client: client} *reply = c.command.Run(env, args.Args) return nil diff --git a/packer/rpc/environment.go b/packer/rpc/environment.go index aaf0db412..1aa3b9abb 100644 --- a/packer/rpc/environment.go +++ b/packer/rpc/environment.go @@ -2,6 +2,7 @@ package rpc import ( "github.com/mitchellh/packer/packer" + "log" "net/rpc" ) @@ -9,12 +10,14 @@ import ( // where the actual environment is executed over an RPC connection. type Environment struct { client *rpc.Client + mux *MuxConn } // A EnvironmentServer wraps a packer.Environment and makes it exportable // as part of a Golang RPC server. type EnvironmentServer struct { env packer.Environment + mux *MuxConn } type EnvironmentCliArgs struct { @@ -22,33 +25,32 @@ type EnvironmentCliArgs struct { } func (e *Environment) Builder(name string) (b packer.Builder, err error) { - var reply string - err = e.client.Call("Environment.Builder", name, &reply) + var streamId uint32 + err = e.client.Call("Environment.Builder", name, &streamId) if err != nil { return } - client, err := rpcDial(reply) + client, err := NewClientWithMux(e.mux, streamId) if err != nil { - return + return nil, err } - - b = Builder(client) + b = client.Builder() return } func (e *Environment) Cache() packer.Cache { - var reply string - if err := e.client.Call("Environment.Cache", new(interface{}), &reply); err != nil { + var streamId uint32 + if err := e.client.Call("Environment.Cache", new(interface{}), &streamId); err != nil { panic(err) } - client, err := rpcDial(reply) + client, err := NewClientWithMux(e.mux, streamId) if err != nil { - panic(err) + log.Printf("[ERR] Error getting cache client: %s", err) + return nil } - - return Cache(client) + return client.Cache() } func (e *Environment) Cli(args []string) (result int, err error) { @@ -58,85 +60,81 @@ func (e *Environment) Cli(args []string) (result int, err error) { } func (e *Environment) Hook(name string) (h packer.Hook, err error) { - var reply string - err = e.client.Call("Environment.Hook", name, &reply) + var streamId uint32 + err = e.client.Call("Environment.Hook", name, &streamId) if err != nil { return } - client, err := rpcDial(reply) + client, err := NewClientWithMux(e.mux, streamId) if err != nil { - return + return nil, err } - - h = Hook(client) - return + return client.Hook(), nil } func (e *Environment) PostProcessor(name string) (p packer.PostProcessor, err error) { - var reply string - err = e.client.Call("Environment.PostProcessor", name, &reply) + var streamId uint32 + err = e.client.Call("Environment.PostProcessor", name, &streamId) if err != nil { return } - client, err := rpcDial(reply) + client, err := NewClientWithMux(e.mux, streamId) if err != nil { - return + return nil, err } - - p = PostProcessor(client) + p = client.PostProcessor() return } func (e *Environment) Provisioner(name string) (p packer.Provisioner, err error) { - var reply string - err = e.client.Call("Environment.Provisioner", name, &reply) + var streamId uint32 + err = e.client.Call("Environment.Provisioner", name, &streamId) if err != nil { return } - client, err := rpcDial(reply) + client, err := NewClientWithMux(e.mux, streamId) if err != nil { - return + return nil, err } - - p = Provisioner(client) + p = client.Provisioner() return } func (e *Environment) Ui() packer.Ui { - var reply string - e.client.Call("Environment.Ui", new(interface{}), &reply) + var streamId uint32 + e.client.Call("Environment.Ui", new(interface{}), &streamId) - client, err := rpcDial(reply) + client, err := NewClientWithMux(e.mux, streamId) if err != nil { - panic(err) + log.Printf("[ERR] Error connecting to Ui: %s", err) + return nil } - - return &Ui{client: client} + return client.Ui() } -func (e *EnvironmentServer) Builder(name *string, reply *string) error { - builder, err := e.env.Builder(*name) +func (e *EnvironmentServer) Builder(name string, reply *uint32) error { + builder, err := e.env.Builder(name) if err != nil { - return err + return NewBasicError(err) } - // Wrap it - server := rpc.NewServer() - RegisterBuilder(server, builder) - - *reply = serveSingleConn(server) + *reply = e.mux.NextId() + server := NewServerWithMux(e.mux, *reply) + server.RegisterBuilder(builder) + go server.Serve() return nil } -func (e *EnvironmentServer) Cache(args *interface{}, reply *string) error { +func (e *EnvironmentServer) Cache(args *interface{}, reply *uint32) error { cache := e.env.Cache() - server := rpc.NewServer() - RegisterCache(server, cache) - *reply = serveSingleConn(server) + *reply = e.mux.NextId() + server := NewServerWithMux(e.mux, *reply) + server.RegisterCache(cache) + go server.Serve() return nil } @@ -145,53 +143,51 @@ func (e *EnvironmentServer) Cli(args *EnvironmentCliArgs, reply *int) (err error return } -func (e *EnvironmentServer) Hook(name *string, reply *string) error { - hook, err := e.env.Hook(*name) +func (e *EnvironmentServer) Hook(name string, reply *uint32) error { + hook, err := e.env.Hook(name) if err != nil { - return err + return NewBasicError(err) } - // Wrap it - server := rpc.NewServer() - RegisterHook(server, hook) - - *reply = serveSingleConn(server) + *reply = e.mux.NextId() + server := NewServerWithMux(e.mux, *reply) + server.RegisterHook(hook) + go server.Serve() return nil } -func (e *EnvironmentServer) PostProcessor(name *string, reply *string) error { - pp, err := e.env.PostProcessor(*name) +func (e *EnvironmentServer) PostProcessor(name string, reply *uint32) error { + pp, err := e.env.PostProcessor(name) if err != nil { - return err + return NewBasicError(err) } - server := rpc.NewServer() - RegisterPostProcessor(server, pp) - - *reply = serveSingleConn(server) + *reply = e.mux.NextId() + server := NewServerWithMux(e.mux, *reply) + server.RegisterPostProcessor(pp) + go server.Serve() return nil } -func (e *EnvironmentServer) Provisioner(name *string, reply *string) error { - prov, err := e.env.Provisioner(*name) +func (e *EnvironmentServer) Provisioner(name string, reply *uint32) error { + prov, err := e.env.Provisioner(name) if err != nil { - return err + return NewBasicError(err) } - server := rpc.NewServer() - RegisterProvisioner(server, prov) - - *reply = serveSingleConn(server) + *reply = e.mux.NextId() + server := NewServerWithMux(e.mux, *reply) + server.RegisterProvisioner(prov) + go server.Serve() return nil } -func (e *EnvironmentServer) Ui(args *interface{}, reply *string) error { +func (e *EnvironmentServer) Ui(args *interface{}, reply *uint32) error { ui := e.env.Ui() - // Wrap it - server := rpc.NewServer() - RegisterUi(server, ui) - - *reply = serveSingleConn(server) + *reply = e.mux.NextId() + server := NewServerWithMux(e.mux, *reply) + server.RegisterUi(ui) + go server.Serve() return nil } diff --git a/packer/rpc/environment_test.go b/packer/rpc/environment_test.go index cb80929ec..bd0a6784a 100644 --- a/packer/rpc/environment_test.go +++ b/packer/rpc/environment_test.go @@ -2,7 +2,6 @@ package rpc import ( "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "testing" ) @@ -69,16 +68,11 @@ func TestEnvironmentRPC(t *testing.T) { e := &testEnvironment{} // Start the server - server := rpc.NewServer() - RegisterEnvironment(server, e) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - eClient := &Environment{client} + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterEnvironment(e) + eClient := client.Environment() // Test Builder builder, _ := eClient.Builder("foo") diff --git a/packer/rpc/server.go b/packer/rpc/server.go index 32190a2fe..838405dd2 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -44,7 +44,7 @@ func RegisterCommunicator(s *rpc.Server, c packer.Communicator) { // Registers the appropriate endpoint on an RPC server to serve a // Packer Environment func RegisterEnvironment(s *rpc.Server, e packer.Environment) { - registerComponent(s, "Environment", &EnvironmentServer{e}, false) + registerComponent(s, "Environment", &EnvironmentServer{env: e}, false) } // Registers the appropriate endpoint on an RPC server to serve a diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index 53e4d0ddd..ac4c6b0d1 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -17,6 +17,7 @@ const ( DefaultCacheEndpoint = "Cache" DefaultCommandEndpoint = "Command" DefaultCommunicatorEndpoint = "Communicator" + DefaultEnvironmentEndpoint = "Environment" DefaultHookEndpoint = "Hook" DefaultPostProcessorEndpoint = "PostProcessor" DefaultProvisionerEndpoint = "Provisioner" @@ -81,6 +82,13 @@ func (s *Server) RegisterCommunicator(c packer.Communicator) { }) } +func (s *Server) RegisterEnvironment(b packer.Environment) { + s.server.RegisterName(DefaultEnvironmentEndpoint, &EnvironmentServer{ + env: b, + mux: s.mux, + }) +} + func (s *Server) RegisterHook(h packer.Hook) { s.server.RegisterName(DefaultHookEndpoint, &HookServer{ hook: h, From a8b056e93936827778ebeb91cf765c8624f7fb45 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 13:18:48 -0800 Subject: [PATCH 24/37] packer/rpc: builds --- packer/rpc/build.go | 49 ++++++++++++++++++++-------------------- packer/rpc/build_test.go | 49 ++++++++++------------------------------ packer/rpc/client.go | 7 ++++++ packer/rpc/server.go | 2 +- packer/rpc/server_new.go | 8 +++++++ 5 files changed, 53 insertions(+), 62 deletions(-) diff --git a/packer/rpc/build.go b/packer/rpc/build.go index f2f7d48ab..91988fae1 100644 --- a/packer/rpc/build.go +++ b/packer/rpc/build.go @@ -9,16 +9,14 @@ import ( // over an RPC connection. type build struct { client *rpc.Client + mux *MuxConn } // BuildServer wraps a packer.Build implementation and makes it exportable // as part of a Golang RPC server. type BuildServer struct { build packer.Build -} - -type BuildRunArgs struct { - UiRPCAddress string + mux *MuxConn } type BuildPrepareResponse struct { @@ -27,7 +25,7 @@ type BuildPrepareResponse struct { } func Build(client *rpc.Client) *build { - return &build{client} + return &build{client: client} } func (b *build) Name() (result string) { @@ -45,25 +43,25 @@ func (b *build) Prepare(v map[string]string) ([]string, error) { } func (b *build) Run(ui packer.Ui, cache packer.Cache) ([]packer.Artifact, error) { - // Create and start the server for the UI - server := rpc.NewServer() - RegisterCache(server, cache) - RegisterUi(server, ui) - args := &BuildRunArgs{serveSingleConn(server)} + nextId := b.mux.NextId() + server := NewServerWithMux(b.mux, nextId) + server.RegisterCache(cache) + server.RegisterUi(ui) + go server.Serve() - var result []string - if err := b.client.Call("Build.Run", args, &result); err != nil { + var result []uint32 + if err := b.client.Call("Build.Run", nextId, &result); err != nil { return nil, err } artifacts := make([]packer.Artifact, len(result)) - for i, addr := range result { - client, err := rpcDial(addr) + for i, streamId := range result { + client, err := NewClientWithMux(b.mux, streamId) if err != nil { return nil, err } - artifacts[i] = Artifact(client) + artifacts[i] = client.Artifact() } return artifacts, nil @@ -101,23 +99,26 @@ func (b *BuildServer) Prepare(v map[string]string, resp *BuildPrepareResponse) e return nil } -func (b *BuildServer) Run(args *BuildRunArgs, reply *[]string) error { - client, err := rpcDial(args.UiRPCAddress) +func (b *BuildServer) Run(streamId uint32, reply *[]uint32) error { + client, err := NewClientWithMux(b.mux, streamId) if err != nil { - return err + return NewBasicError(err) } + defer client.Close() - ui := &Ui{client: client} - artifacts, err := b.build.Run(ui, Cache(client)) + artifacts, err := b.build.Run(client.Ui(), client.Cache()) if err != nil { return NewBasicError(err) } - *reply = make([]string, len(artifacts)) + *reply = make([]uint32, len(artifacts)) for i, artifact := range artifacts { - server := rpc.NewServer() - RegisterArtifact(server, artifact) - (*reply)[i] = serveSingleConn(server) + streamId := b.mux.NextId() + server := NewServerWithMux(b.mux, streamId) + server.RegisterArtifact(artifact) + go server.Serve() + + (*reply)[i] = streamId } return nil diff --git a/packer/rpc/build_test.go b/packer/rpc/build_test.go index 60c2c0111..ff79ee49e 100644 --- a/packer/rpc/build_test.go +++ b/packer/rpc/build_test.go @@ -3,7 +3,6 @@ package rpc import ( "errors" "github.com/mitchellh/packer/packer" - "net/rpc" "reflect" "testing" ) @@ -60,25 +59,13 @@ func (b *testBuild) Cancel() { b.cancelCalled = true } -func buildRPCClient(t *testing.T) (*testBuild, packer.Build) { - // Create the interface to test - b := new(testBuild) - - // Start the server - server := rpc.NewServer() - RegisterBuild(server, b) - address := serveSingleConn(server) - - // Create the client over RPC and run some methods to verify it works - client, err := rpc.Dial("tcp", address) - if err != nil { - t.Fatalf("err: %s", err) - } - return b, Build(client) -} - func TestBuild(t *testing.T) { - b, bClient := buildRPCClient(t) + b := new(testBuild) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuild(b) + bClient := client.Build() // Test Name bClient.Name() @@ -120,23 +107,6 @@ func TestBuild(t *testing.T) { t.Fatalf("bad: %#v", artifacts) } - // Test the UI given to run, which should be fully functional - if b.runCalled { - b.runCache.Lock("foo") - if !cache.lockCalled { - t.Fatal("lock shuld be called") - } - - b.runUi.Say("format") - if !ui.sayCalled { - t.Fatal("say should be called") - } - - if ui.sayMessage != "format" { - t.Fatalf("bad: %#v", ui.sayMessage) - } - } - // Test run with an error b.errRunResult = true _, err = bClient.Run(ui, cache) @@ -164,7 +134,12 @@ func TestBuild(t *testing.T) { } func TestBuildPrepare_Warnings(t *testing.T) { - b, bClient := buildRPCClient(t) + b := new(testBuild) + client, server := testClientServer(t) + defer client.Close() + defer server.Close() + server.RegisterBuild(b) + bClient := client.Build() expected := []string{"foo"} b.prepareWarnings = expected diff --git a/packer/rpc/client.go b/packer/rpc/client.go index 237cb0c88..c9ddc5f57 100644 --- a/packer/rpc/client.go +++ b/packer/rpc/client.go @@ -45,6 +45,13 @@ func (c *Client) Artifact() packer.Artifact { } } +func (c *Client) Build() packer.Build { + return &build{ + client: c.client, + mux: c.mux, + } +} + func (c *Client) Builder() packer.Builder { return &builder{ client: c.client, diff --git a/packer/rpc/server.go b/packer/rpc/server.go index 838405dd2..5a08f6a73 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -14,7 +14,7 @@ func RegisterArtifact(s *rpc.Server, a packer.Artifact) { // Registers the appropriate endpoint on an RPC server to serve a // Packer Build. func RegisterBuild(s *rpc.Server, b packer.Build) { - registerComponent(s, "Build", &BuildServer{b}, false) + registerComponent(s, "Build", &BuildServer{build: b}, false) } // Registers the appropriate endpoint on an RPC server to serve a diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go index ac4c6b0d1..bf194bd6c 100644 --- a/packer/rpc/server_new.go +++ b/packer/rpc/server_new.go @@ -13,6 +13,7 @@ var endpointId uint64 const ( DefaultArtifactEndpoint string = "Artifact" + DefaultBuildEndpoint = "Build" DefaultBuilderEndpoint = "Builder" DefaultCacheEndpoint = "Cache" DefaultCommandEndpoint = "Command" @@ -55,6 +56,13 @@ func (s *Server) RegisterArtifact(a packer.Artifact) { }) } +func (s *Server) RegisterBuild(b packer.Build) { + s.server.RegisterName(DefaultBuildEndpoint, &BuildServer{ + build: b, + mux: s.mux, + }) +} + func (s *Server) RegisterBuilder(b packer.Builder) { s.server.RegisterName(DefaultBuilderEndpoint, &BuilderServer{ builder: b, From 8d4ba1fc2bbef53245549fc3c7a524bc7003cf2b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 13:23:07 -0800 Subject: [PATCH 25/37] packer/rpc: complete command --- packer/rpc/command.go | 23 +++++++++++++---------- packer/rpc/command_test.go | 5 ----- packer/rpc/post_processor.go | 4 ---- packer/rpc/post_processor_test.go | 2 +- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/packer/rpc/command.go b/packer/rpc/command.go index 945b8d58b..6d70fdace 100644 --- a/packer/rpc/command.go +++ b/packer/rpc/command.go @@ -20,8 +20,8 @@ type CommandServer struct { } type CommandRunArgs struct { - RPCAddress string Args []string + StreamId uint32 } type CommandSynopsisArgs byte @@ -40,11 +40,15 @@ func (c *command) Help() (result string) { } func (c *command) Run(env packer.Environment, args []string) (result int) { - // Create and start the server for the Environment - server := rpc.NewServer() - RegisterEnvironment(server, env) + nextId := c.mux.NextId() + server := NewServerWithMux(c.mux, nextId) + server.RegisterEnvironment(env) + go server.Serve() - rpcArgs := &CommandRunArgs{serveSingleConn(server), args} + rpcArgs := &CommandRunArgs{ + Args: args, + StreamId: nextId, + } err := c.client.Call("Command.Run", rpcArgs, &result) if err != nil { panic(err) @@ -68,14 +72,13 @@ func (c *CommandServer) Help(args *interface{}, reply *string) error { } func (c *CommandServer) Run(args *CommandRunArgs, reply *int) error { - client, err := rpcDial(args.RPCAddress) + client, err := NewClientWithMux(c.mux, args.StreamId) if err != nil { - return err + return NewBasicError(err) } + defer client.Close() - env := &Environment{client: client} - - *reply = c.command.Run(env, args.Args) + *reply = c.command.Run(client.Environment(), args.Args) return nil } diff --git a/packer/rpc/command_test.go b/packer/rpc/command_test.go index 976b70e6f..a6c3c9592 100644 --- a/packer/rpc/command_test.go +++ b/packer/rpc/command_test.go @@ -59,11 +59,6 @@ func TestRPCCommand(t *testing.T) { t.Fatal("runEnv should not be nil") } - command.runEnv.Ui() - if !testEnv.uiCalled { - t.Fatal("ui should be called") - } - // Test Synopsis synopsis := commClient.Synopsis() if synopsis != "foo" { diff --git a/packer/rpc/post_processor.go b/packer/rpc/post_processor.go index 2a0dfe62a..ad8fd0b9d 100644 --- a/packer/rpc/post_processor.go +++ b/packer/rpc/post_processor.go @@ -30,10 +30,6 @@ type PostProcessorProcessResponse struct { StreamId uint32 } -func PostProcessor(client *rpc.Client) *postProcessor { - return &postProcessor{client: client} -} - func (p *postProcessor) Configure(raw ...interface{}) (err error) { args := &PostProcessorConfigureArgs{Configs: raw} if cerr := p.client.Call("PostProcessor.Configure", args, &err); cerr != nil { diff --git a/packer/rpc/post_processor_test.go b/packer/rpc/post_processor_test.go index d469ee4e0..dabd8c03a 100644 --- a/packer/rpc/post_processor_test.go +++ b/packer/rpc/post_processor_test.go @@ -83,7 +83,7 @@ func TestPostProcessorRPC(t *testing.T) { func TestPostProcessor_Implements(t *testing.T) { var raw interface{} - raw = PostProcessor(nil) + raw = new(postProcessor) if _, ok := raw.(packer.PostProcessor); !ok { t.Fatal("not a postprocessor") } From ce2304c9485af3adb6f2bb70c4a9c24bba8e5bb4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 13:26:07 -0800 Subject: [PATCH 26/37] packer/rpc: remove unused methods --- packer/rpc/artifact.go | 4 - packer/rpc/artifact_test.go | 2 +- packer/rpc/build.go | 4 - packer/rpc/build_test.go | 2 +- packer/rpc/builder.go | 4 - packer/rpc/builder_test.go | 2 +- packer/rpc/cache.go | 4 - packer/rpc/cache_test.go | 6 +- packer/rpc/command.go | 4 - packer/rpc/command_test.go | 2 +- packer/rpc/hook.go | 4 - packer/rpc/provisioner.go | 3 - packer/rpc/provisioner_test.go | 2 +- packer/rpc/server.go | 183 ++++++++++++++++++++++----------- packer/rpc/server_new.go | 155 ---------------------------- 15 files changed, 131 insertions(+), 250 deletions(-) delete mode 100644 packer/rpc/server_new.go diff --git a/packer/rpc/artifact.go b/packer/rpc/artifact.go index 8ca3ef869..c6e0a208d 100644 --- a/packer/rpc/artifact.go +++ b/packer/rpc/artifact.go @@ -18,10 +18,6 @@ type ArtifactServer struct { artifact packer.Artifact } -func Artifact(client *rpc.Client) *artifact { - return &artifact{client: client} -} - func (a *artifact) BuilderId() (result string) { a.client.Call(a.endpoint+".BuilderId", new(interface{}), &result) return diff --git a/packer/rpc/artifact_test.go b/packer/rpc/artifact_test.go index d5405ab6e..37f4cef9f 100644 --- a/packer/rpc/artifact_test.go +++ b/packer/rpc/artifact_test.go @@ -37,5 +37,5 @@ func TestArtifactRPC(t *testing.T) { } func TestArtifact_Implements(t *testing.T) { - var _ packer.Artifact = Artifact(nil) + var _ packer.Artifact = new(artifact) } diff --git a/packer/rpc/build.go b/packer/rpc/build.go index 91988fae1..f63f35f76 100644 --- a/packer/rpc/build.go +++ b/packer/rpc/build.go @@ -24,10 +24,6 @@ type BuildPrepareResponse struct { Error error } -func Build(client *rpc.Client) *build { - return &build{client: client} -} - func (b *build) Name() (result string) { b.client.Call("Build.Name", new(interface{}), &result) return diff --git a/packer/rpc/build_test.go b/packer/rpc/build_test.go index ff79ee49e..6c18ad384 100644 --- a/packer/rpc/build_test.go +++ b/packer/rpc/build_test.go @@ -154,5 +154,5 @@ func TestBuildPrepare_Warnings(t *testing.T) { } func TestBuild_ImplementsBuild(t *testing.T) { - var _ packer.Build = Build(nil) + var _ packer.Build = new(build) } diff --git a/packer/rpc/builder.go b/packer/rpc/builder.go index 20c166a58..7721414e2 100644 --- a/packer/rpc/builder.go +++ b/packer/rpc/builder.go @@ -29,10 +29,6 @@ type BuilderPrepareResponse struct { Error error } -func Builder(client *rpc.Client) *builder { - return &builder{client: client} -} - func (b *builder) Prepare(config ...interface{}) ([]string, error) { var resp BuilderPrepareResponse cerr := b.client.Call("Builder.Prepare", &BuilderPrepareArgs{config}, &resp) diff --git a/packer/rpc/builder_test.go b/packer/rpc/builder_test.go index 22bbeea06..37e8041bb 100644 --- a/packer/rpc/builder_test.go +++ b/packer/rpc/builder_test.go @@ -141,5 +141,5 @@ func TestBuilderCancel(t *testing.T) { } func TestBuilder_ImplementsBuilder(t *testing.T) { - var _ packer.Builder = Builder(nil) + var _ packer.Builder = new(builder) } diff --git a/packer/rpc/cache.go b/packer/rpc/cache.go index 73b154cb6..184286411 100644 --- a/packer/rpc/cache.go +++ b/packer/rpc/cache.go @@ -17,10 +17,6 @@ type CacheServer struct { cache packer.Cache } -func Cache(client *rpc.Client) *cache { - return &cache{client} -} - type CacheRLockResponse struct { Path string Exists bool diff --git a/packer/rpc/cache_test.go b/packer/rpc/cache_test.go index a9a1f110f..702ca98e4 100644 --- a/packer/rpc/cache_test.go +++ b/packer/rpc/cache_test.go @@ -39,11 +39,7 @@ func (t *testCache) RUnlock(key string) { } func TestCache_Implements(t *testing.T) { - var raw interface{} - raw = Cache(nil) - if _, ok := raw.(packer.Cache); !ok { - t.Fatal("Cache must be a cache.") - } + var _ packer.Cache = new(cache) } func TestCacheRPC(t *testing.T) { diff --git a/packer/rpc/command.go b/packer/rpc/command.go index 6d70fdace..e0243e9bb 100644 --- a/packer/rpc/command.go +++ b/packer/rpc/command.go @@ -26,10 +26,6 @@ type CommandRunArgs struct { type CommandSynopsisArgs byte -func Command(client *rpc.Client) *command { - return &command{client: client} -} - func (c *command) Help() (result string) { err := c.client.Call("Command.Help", new(interface{}), &result) if err != nil { diff --git a/packer/rpc/command_test.go b/packer/rpc/command_test.go index a6c3c9592..7ba433956 100644 --- a/packer/rpc/command_test.go +++ b/packer/rpc/command_test.go @@ -67,5 +67,5 @@ func TestRPCCommand(t *testing.T) { } func TestCommand_Implements(t *testing.T) { - var _ packer.Command = Command(nil) + var _ packer.Command = new(command) } diff --git a/packer/rpc/hook.go b/packer/rpc/hook.go index 8162dd4f2..18682fac6 100644 --- a/packer/rpc/hook.go +++ b/packer/rpc/hook.go @@ -26,10 +26,6 @@ type HookRunArgs struct { StreamId uint32 } -func Hook(client *rpc.Client) *hook { - return &hook{client: client} -} - func (h *hook) Run(name string, ui packer.Ui, comm packer.Communicator, data interface{}) error { nextId := h.mux.NextId() server := NewServerWithMux(h.mux, nextId) diff --git a/packer/rpc/provisioner.go b/packer/rpc/provisioner.go index eafd65756..7ddb32ec3 100644 --- a/packer/rpc/provisioner.go +++ b/packer/rpc/provisioner.go @@ -24,9 +24,6 @@ type ProvisionerPrepareArgs struct { Configs []interface{} } -func Provisioner(client *rpc.Client) *provisioner { - return &provisioner{client: client} -} func (p *provisioner) Prepare(configs ...interface{}) (err error) { args := &ProvisionerPrepareArgs{configs} if cerr := p.client.Call("Provisioner.Prepare", args, &err); cerr != nil { diff --git a/packer/rpc/provisioner_test.go b/packer/rpc/provisioner_test.go index 5d1604b74..a17e0694c 100644 --- a/packer/rpc/provisioner_test.go +++ b/packer/rpc/provisioner_test.go @@ -43,5 +43,5 @@ func TestProvisionerRPC(t *testing.T) { } func TestProvisioner_Implements(t *testing.T) { - var _ packer.Provisioner = Provisioner(nil) + var _ packer.Provisioner = new(provisioner) } diff --git a/packer/rpc/server.go b/packer/rpc/server.go index 5a08f6a73..bf194bd6c 100644 --- a/packer/rpc/server.go +++ b/packer/rpc/server.go @@ -1,88 +1,155 @@ package rpc import ( + "fmt" "github.com/mitchellh/packer/packer" + "io" + "log" "net/rpc" + "sync/atomic" ) -// Registers the appropriate endpoint on an RPC server to serve an -// Artifact. -func RegisterArtifact(s *rpc.Server, a packer.Artifact) { - registerComponent(s, "Artifact", &ArtifactServer{a}, false) +var endpointId uint64 + +const ( + DefaultArtifactEndpoint string = "Artifact" + DefaultBuildEndpoint = "Build" + DefaultBuilderEndpoint = "Builder" + DefaultCacheEndpoint = "Cache" + DefaultCommandEndpoint = "Command" + DefaultCommunicatorEndpoint = "Communicator" + DefaultEnvironmentEndpoint = "Environment" + DefaultHookEndpoint = "Hook" + DefaultPostProcessorEndpoint = "PostProcessor" + DefaultProvisionerEndpoint = "Provisioner" + DefaultUiEndpoint = "Ui" +) + +// Server represents an RPC server for Packer. This must be paired on +// the other side with a Client. +type Server struct { + mux *MuxConn + streamId uint32 + server *rpc.Server } -// Registers the appropriate endpoint on an RPC server to serve a -// Packer Build. -func RegisterBuild(s *rpc.Server, b packer.Build) { - registerComponent(s, "Build", &BuildServer{build: b}, false) +// NewServer returns a new Packer RPC server. +func NewServer(conn io.ReadWriteCloser) *Server { + return NewServerWithMux(NewMuxConn(conn), 0) } -// Registers the appropriate endpoint on an RPC server to serve a -// Packer Builder. -func RegisterBuilder(s *rpc.Server, b packer.Builder) { - registerComponent(s, "Builder", &BuilderServer{builder: b}, false) +func NewServerWithMux(mux *MuxConn, streamId uint32) *Server { + return &Server{ + mux: mux, + streamId: streamId, + server: rpc.NewServer(), + } } -// Registers the appropriate endpoint on an RPC server to serve a -// Packer Cache. -func RegisterCache(s *rpc.Server, c packer.Cache) { - registerComponent(s, "Cache", &CacheServer{c}, false) +func (s *Server) Close() error { + return s.mux.Close() } -// Registers the appropriate endpoint on an RPC server to serve a -// Packer Command. -func RegisterCommand(s *rpc.Server, c packer.Command) { - registerComponent(s, "Command", &CommandServer{command: c}, false) +func (s *Server) RegisterArtifact(a packer.Artifact) { + s.server.RegisterName(DefaultArtifactEndpoint, &ArtifactServer{ + artifact: a, + }) } -// Registers the appropriate endpoint on an RPC server to serve a -// Packer Communicator. -func RegisterCommunicator(s *rpc.Server, c packer.Communicator) { - registerComponent(s, "Communicator", &CommunicatorServer{c: c}, false) +func (s *Server) RegisterBuild(b packer.Build) { + s.server.RegisterName(DefaultBuildEndpoint, &BuildServer{ + build: b, + mux: s.mux, + }) } -// Registers the appropriate endpoint on an RPC server to serve a -// Packer Environment -func RegisterEnvironment(s *rpc.Server, e packer.Environment) { - registerComponent(s, "Environment", &EnvironmentServer{env: e}, false) +func (s *Server) RegisterBuilder(b packer.Builder) { + s.server.RegisterName(DefaultBuilderEndpoint, &BuilderServer{ + builder: b, + mux: s.mux, + }) } -// Registers the appropriate endpoint on an RPC server to serve a -// Hook. -func RegisterHook(s *rpc.Server, h packer.Hook) { - registerComponent(s, "Hook", &HookServer{hook: h}, false) +func (s *Server) RegisterCache(c packer.Cache) { + s.server.RegisterName(DefaultCacheEndpoint, &CacheServer{ + cache: c, + }) } -// Registers the appropriate endpoing on an RPC server to serve a -// PostProcessor. -func RegisterPostProcessor(s *rpc.Server, p packer.PostProcessor) { - registerComponent(s, "PostProcessor", &PostProcessorServer{p: p}, false) +func (s *Server) RegisterCommand(c packer.Command) { + s.server.RegisterName(DefaultCommandEndpoint, &CommandServer{ + command: c, + mux: s.mux, + }) } -// Registers the appropriate endpoint on an RPC server to serve a packer.Provisioner -func RegisterProvisioner(s *rpc.Server, p packer.Provisioner) { - registerComponent(s, "Provisioner", &ProvisionerServer{p: p}, false) +func (s *Server) RegisterCommunicator(c packer.Communicator) { + s.server.RegisterName(DefaultCommunicatorEndpoint, &CommunicatorServer{ + c: c, + mux: s.mux, + }) } -// Registers the appropriate endpoint on an RPC server to serve a -// Packer UI -func RegisterUi(s *rpc.Server, ui packer.Ui) { - registerComponent(s, "Ui", &UiServer{ui}, false) +func (s *Server) RegisterEnvironment(b packer.Environment) { + s.server.RegisterName(DefaultEnvironmentEndpoint, &EnvironmentServer{ + env: b, + mux: s.mux, + }) } -func serveSingleConn(s *rpc.Server) string { - l := netListenerInRange(portRangeMin, portRangeMax) - - // Accept a single connection in a goroutine and then exit - go func() { - defer l.Close() - conn, err := l.Accept() - if err != nil { - panic(err) - } - - s.ServeConn(conn) - }() - - return l.Addr().String() +func (s *Server) RegisterHook(h packer.Hook) { + s.server.RegisterName(DefaultHookEndpoint, &HookServer{ + hook: h, + mux: s.mux, + }) +} + +func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { + s.server.RegisterName(DefaultPostProcessorEndpoint, &PostProcessorServer{ + mux: s.mux, + p: p, + }) +} + +func (s *Server) RegisterProvisioner(p packer.Provisioner) { + s.server.RegisterName(DefaultProvisionerEndpoint, &ProvisionerServer{ + mux: s.mux, + p: p, + }) +} + +func (s *Server) RegisterUi(ui packer.Ui) { + s.server.RegisterName(DefaultUiEndpoint, &UiServer{ + ui: ui, + }) +} + +// ServeConn serves a single connection over the RPC server. It is up +// to the caller to obtain a proper io.ReadWriteCloser. +func (s *Server) Serve() { + // Accept a connection on stream ID 0, which is always used for + // normal client to server connections. + stream, err := s.mux.Accept(s.streamId) + defer stream.Close() + if err != nil { + log.Printf("[ERR] Error retrieving stream for serving: %s", err) + return + } + + s.server.ServeConn(stream) +} + +// registerComponent registers a single Packer RPC component onto +// the RPC server. If id is true, then a unique ID number will be appended +// onto the end of the endpoint. +// +// The endpoint name is returned. +func registerComponent(server *rpc.Server, name string, rcvr interface{}, id bool) string { + endpoint := name + if id { + fmt.Sprintf("%s.%d", endpoint, atomic.AddUint64(&endpointId, 1)) + } + + server.RegisterName(endpoint, rcvr) + return endpoint } diff --git a/packer/rpc/server_new.go b/packer/rpc/server_new.go deleted file mode 100644 index bf194bd6c..000000000 --- a/packer/rpc/server_new.go +++ /dev/null @@ -1,155 +0,0 @@ -package rpc - -import ( - "fmt" - "github.com/mitchellh/packer/packer" - "io" - "log" - "net/rpc" - "sync/atomic" -) - -var endpointId uint64 - -const ( - DefaultArtifactEndpoint string = "Artifact" - DefaultBuildEndpoint = "Build" - DefaultBuilderEndpoint = "Builder" - DefaultCacheEndpoint = "Cache" - DefaultCommandEndpoint = "Command" - DefaultCommunicatorEndpoint = "Communicator" - DefaultEnvironmentEndpoint = "Environment" - DefaultHookEndpoint = "Hook" - DefaultPostProcessorEndpoint = "PostProcessor" - DefaultProvisionerEndpoint = "Provisioner" - DefaultUiEndpoint = "Ui" -) - -// Server represents an RPC server for Packer. This must be paired on -// the other side with a Client. -type Server struct { - mux *MuxConn - streamId uint32 - server *rpc.Server -} - -// NewServer returns a new Packer RPC server. -func NewServer(conn io.ReadWriteCloser) *Server { - return NewServerWithMux(NewMuxConn(conn), 0) -} - -func NewServerWithMux(mux *MuxConn, streamId uint32) *Server { - return &Server{ - mux: mux, - streamId: streamId, - server: rpc.NewServer(), - } -} - -func (s *Server) Close() error { - return s.mux.Close() -} - -func (s *Server) RegisterArtifact(a packer.Artifact) { - s.server.RegisterName(DefaultArtifactEndpoint, &ArtifactServer{ - artifact: a, - }) -} - -func (s *Server) RegisterBuild(b packer.Build) { - s.server.RegisterName(DefaultBuildEndpoint, &BuildServer{ - build: b, - mux: s.mux, - }) -} - -func (s *Server) RegisterBuilder(b packer.Builder) { - s.server.RegisterName(DefaultBuilderEndpoint, &BuilderServer{ - builder: b, - mux: s.mux, - }) -} - -func (s *Server) RegisterCache(c packer.Cache) { - s.server.RegisterName(DefaultCacheEndpoint, &CacheServer{ - cache: c, - }) -} - -func (s *Server) RegisterCommand(c packer.Command) { - s.server.RegisterName(DefaultCommandEndpoint, &CommandServer{ - command: c, - mux: s.mux, - }) -} - -func (s *Server) RegisterCommunicator(c packer.Communicator) { - s.server.RegisterName(DefaultCommunicatorEndpoint, &CommunicatorServer{ - c: c, - mux: s.mux, - }) -} - -func (s *Server) RegisterEnvironment(b packer.Environment) { - s.server.RegisterName(DefaultEnvironmentEndpoint, &EnvironmentServer{ - env: b, - mux: s.mux, - }) -} - -func (s *Server) RegisterHook(h packer.Hook) { - s.server.RegisterName(DefaultHookEndpoint, &HookServer{ - hook: h, - mux: s.mux, - }) -} - -func (s *Server) RegisterPostProcessor(p packer.PostProcessor) { - s.server.RegisterName(DefaultPostProcessorEndpoint, &PostProcessorServer{ - mux: s.mux, - p: p, - }) -} - -func (s *Server) RegisterProvisioner(p packer.Provisioner) { - s.server.RegisterName(DefaultProvisionerEndpoint, &ProvisionerServer{ - mux: s.mux, - p: p, - }) -} - -func (s *Server) RegisterUi(ui packer.Ui) { - s.server.RegisterName(DefaultUiEndpoint, &UiServer{ - ui: ui, - }) -} - -// ServeConn serves a single connection over the RPC server. It is up -// to the caller to obtain a proper io.ReadWriteCloser. -func (s *Server) Serve() { - // Accept a connection on stream ID 0, which is always used for - // normal client to server connections. - stream, err := s.mux.Accept(s.streamId) - defer stream.Close() - if err != nil { - log.Printf("[ERR] Error retrieving stream for serving: %s", err) - return - } - - s.server.ServeConn(stream) -} - -// registerComponent registers a single Packer RPC component onto -// the RPC server. If id is true, then a unique ID number will be appended -// onto the end of the endpoint. -// -// The endpoint name is returned. -func registerComponent(server *rpc.Server, name string, rcvr interface{}, id bool) string { - endpoint := name - if id { - fmt.Sprintf("%s.%d", endpoint, atomic.AddUint64(&endpointId, 1)) - } - - server.RegisterName(endpoint, rcvr) - return endpoint -} From b4567c63804c51c6f9443786cba0962f4e363ca4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 13:47:18 -0800 Subject: [PATCH 27/37] packer/plugin: use new RPC API --- packer/plugin/client.go | 31 +++++++------ packer/plugin/plugin.go | 90 ++++-------------------------------- packer/plugin/plugin_test.go | 40 ++++++++++++++-- 3 files changed, 62 insertions(+), 99 deletions(-) diff --git a/packer/plugin/client.go b/packer/plugin/client.go index 792f9d99d..3fefca15f 100644 --- a/packer/plugin/client.go +++ b/packer/plugin/client.go @@ -10,7 +10,6 @@ import ( "io/ioutil" "log" "net" - "net/rpc" "os" "os/exec" "strings" @@ -130,56 +129,56 @@ func (c *Client) Exited() bool { // Returns a builder implementation that is communicating over this // client. If the client hasn't been started, this will start it. func (c *Client) Builder() (packer.Builder, error) { - client, err := c.rpcClient() + client, err := c.packrpcClient() if err != nil { return nil, err } - return &cmdBuilder{packrpc.Builder(client), c}, nil + return &cmdBuilder{client.Builder(), c}, nil } // Returns a command implementation that is communicating over this // client. If the client hasn't been started, this will start it. func (c *Client) Command() (packer.Command, error) { - client, err := c.rpcClient() + client, err := c.packrpcClient() if err != nil { return nil, err } - return &cmdCommand{packrpc.Command(client), c}, nil + return &cmdCommand{client.Command(), c}, nil } // Returns a hook implementation that is communicating over this // client. If the client hasn't been started, this will start it. func (c *Client) Hook() (packer.Hook, error) { - client, err := c.rpcClient() + client, err := c.packrpcClient() if err != nil { return nil, err } - return &cmdHook{packrpc.Hook(client), c}, nil + return &cmdHook{client.Hook(), c}, nil } // Returns a post-processor implementation that is communicating over // this client. If the client hasn't been started, this will start it. func (c *Client) PostProcessor() (packer.PostProcessor, error) { - client, err := c.rpcClient() + client, err := c.packrpcClient() if err != nil { return nil, err } - return &cmdPostProcessor{packrpc.PostProcessor(client), c}, nil + return &cmdPostProcessor{client.PostProcessor(), c}, nil } // Returns a provisioner implementation that is communicating over this // client. If the client hasn't been started, this will start it. func (c *Client) Provisioner() (packer.Provisioner, error) { - client, err := c.rpcClient() + client, err := c.packrpcClient() if err != nil { return nil, err } - return &cmdProvisioner{packrpc.Provisioner(client), c}, nil + return &cmdProvisioner{client.Provisioner(), c}, nil } // End the executing subprocess (if it is running) and perform any cleanup @@ -361,7 +360,7 @@ func (c *Client) logStderr(r io.Reader) { close(c.doneLogging) } -func (c *Client) rpcClient() (*rpc.Client, error) { +func (c *Client) packrpcClient() (*packrpc.Client, error) { address, err := c.Start() if err != nil { return nil, err @@ -376,5 +375,11 @@ func (c *Client) rpcClient() (*rpc.Client, error) { tcpConn := conn.(*net.TCPConn) tcpConn.SetKeepAlive(true) - return rpc.NewClient(tcpConn), nil + client, err := packrpc.NewClient(tcpConn) + if err != nil { + tcpConn.Close() + return nil, err + } + + return client, nil } diff --git a/packer/plugin/plugin.go b/packer/plugin/plugin.go index a91fcc3ce..91aa91274 100644 --- a/packer/plugin/plugin.go +++ b/packer/plugin/plugin.go @@ -14,7 +14,6 @@ import ( packrpc "github.com/mitchellh/packer/packer/rpc" "log" "net" - "net/rpc" "os" "os/signal" "runtime" @@ -35,13 +34,14 @@ const MagicCookieValue = "d602bf8f470bc67ca7faa0386276bbdd4330efaf76d1a219cb4d69 // know how to speak it. const APIVersion = "1" -// This serves a single RPC connection on the given RPC server on -// a random port. -func serve(server *rpc.Server) (err error) { +// Server waits for a connection to this plugin and returns a Packer +// RPC server that you can use to register components and serve them. +func Server() (*packrpc.Server, error) { log.Printf("Plugin build against Packer '%s'", packer.GitCommit) if os.Getenv(MagicCookieKey) != MagicCookieValue { - return errors.New("Please do not execute plugins directly. Packer will execute these for you.") + return nil, errors.New( + "Please do not execute plugins directly. Packer will execute these for you.") } // If there is no explicit number of Go threads to use, then set it @@ -51,12 +51,12 @@ func serve(server *rpc.Server) (err error) { minPort, err := strconv.ParseInt(os.Getenv("PACKER_PLUGIN_MIN_PORT"), 10, 32) if err != nil { - return + return nil, err } maxPort, err := strconv.ParseInt(os.Getenv("PACKER_PLUGIN_MAX_PORT"), 10, 32) if err != nil { - return + return nil, err } log.Printf("Plugin minimum port: %d\n", minPort) @@ -77,7 +77,6 @@ func serve(server *rpc.Server) (err error) { break } - defer listener.Close() // Output the address to stdout @@ -90,13 +89,12 @@ func serve(server *rpc.Server) (err error) { conn, err := listener.Accept() if err != nil { log.Printf("Error accepting connection: %s\n", err.Error()) - return + return nil, err } // Serve a single connection log.Println("Serving a plugin connection...") - server.ServeConn(conn) - return + return packrpc.NewServer(conn), nil } // Registers a signal handler to swallow and count interrupts so that the @@ -115,76 +113,6 @@ func countInterrupts() { }() } -// Serves a builder from a plugin. -func ServeBuilder(builder packer.Builder) { - log.Println("Preparing to serve a builder plugin...") - - server := rpc.NewServer() - packrpc.RegisterBuilder(server, builder) - - countInterrupts() - if err := serve(server); err != nil { - log.Printf("ERROR: %s", err) - os.Exit(1) - } -} - -// Serves a command from a plugin. -func ServeCommand(command packer.Command) { - log.Println("Preparing to serve a command plugin...") - - server := rpc.NewServer() - packrpc.RegisterCommand(server, command) - - countInterrupts() - if err := serve(server); err != nil { - log.Printf("ERROR: %s", err) - os.Exit(1) - } -} - -// Serves a hook from a plugin. -func ServeHook(hook packer.Hook) { - log.Println("Preparing to serve a hook plugin...") - - server := rpc.NewServer() - packrpc.RegisterHook(server, hook) - - countInterrupts() - if err := serve(server); err != nil { - log.Printf("ERROR: %s", err) - os.Exit(1) - } -} - -// Serves a post-processor from a plugin. -func ServePostProcessor(p packer.PostProcessor) { - log.Println("Preparing to serve a post-processor plugin...") - - server := rpc.NewServer() - packrpc.RegisterPostProcessor(server, p) - - countInterrupts() - if err := serve(server); err != nil { - log.Printf("ERROR: %s", err) - os.Exit(1) - } -} - -// Serves a provisioner from a plugin. -func ServeProvisioner(p packer.Provisioner) { - log.Println("Preparing to serve a provisioner plugin...") - - server := rpc.NewServer() - packrpc.RegisterProvisioner(server, p) - - countInterrupts() - if err := serve(server); err != nil { - log.Printf("ERROR: %s", err) - os.Exit(1) - } -} - // Tests whether or not the plugin was interrupted or not. func Interrupted() bool { return atomic.LoadInt32(&Interrupts) > 0 diff --git a/packer/plugin/plugin_test.go b/packer/plugin/plugin_test.go index d80aceaf7..733190ec3 100644 --- a/packer/plugin/plugin_test.go +++ b/packer/plugin/plugin_test.go @@ -54,20 +54,50 @@ func TestHelperProcess(*testing.T) { fmt.Printf("%s1|:1234\n", APIVersion) <-make(chan int) case "builder": - ServeBuilder(new(packer.MockBuilder)) + server, err := Server() + if err != nil { + log.Printf("[ERR] %s", err) + os.Exit(1) + } + server.RegisterBuilder(new(packer.MockBuilder)) + server.Serve() case "command": - ServeCommand(new(helperCommand)) + server, err := Server() + if err != nil { + log.Printf("[ERR] %s", err) + os.Exit(1) + } + server.RegisterCommand(new(helperCommand)) + server.Serve() case "hook": - ServeHook(new(packer.MockHook)) + server, err := Server() + if err != nil { + log.Printf("[ERR] %s", err) + os.Exit(1) + } + server.RegisterHook(new(packer.MockHook)) + server.Serve() case "invalid-rpc-address": fmt.Println("lolinvalid") case "mock": fmt.Printf("%s|:1234\n", APIVersion) <-make(chan int) case "post-processor": - ServePostProcessor(new(helperPostProcessor)) + server, err := Server() + if err != nil { + log.Printf("[ERR] %s", err) + os.Exit(1) + } + server.RegisterPostProcessor(new(helperPostProcessor)) + server.Serve() case "provisioner": - ServeProvisioner(new(packer.MockProvisioner)) + server, err := Server() + if err != nil { + log.Printf("[ERR] %s", err) + os.Exit(1) + } + server.RegisterProvisioner(new(packer.MockProvisioner)) + server.Serve() case "start-timeout": time.Sleep(1 * time.Minute) os.Exit(1) From 82bf5fc79e732f6da62fcbeca799116ee4311ca3 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 13:59:38 -0800 Subject: [PATCH 28/37] plugin/*: update to latest API --- plugin/builder-amazon-chroot/main.go | 7 ++++++- plugin/builder-amazon-ebs/main.go | 7 ++++++- plugin/builder-amazon-instance/main.go | 7 ++++++- plugin/builder-digitalocean/main.go | 7 ++++++- plugin/builder-docker/main.go | 7 ++++++- plugin/builder-openstack/main.go | 7 ++++++- plugin/builder-qemu/main.go | 7 ++++++- plugin/builder-virtualbox/main.go | 7 ++++++- plugin/builder-vmware/main.go | 7 ++++++- plugin/command-build/main.go | 7 ++++++- plugin/command-fix/main.go | 7 ++++++- plugin/command-inspect/main.go | 7 ++++++- plugin/command-validate/main.go | 7 ++++++- plugin/post-processor-vagrant/main.go | 7 ++++++- plugin/post-processor-vsphere/main.go | 7 ++++++- plugin/provisioner-ansible-local/main.go | 7 ++++++- plugin/provisioner-chef-solo/main.go | 7 ++++++- plugin/provisioner-file/main.go | 7 ++++++- plugin/provisioner-puppet-masterless/main.go | 7 ++++++- plugin/provisioner-salt-masterless/main.go | 7 ++++++- plugin/provisioner-shell/main.go | 7 ++++++- 21 files changed, 126 insertions(+), 21 deletions(-) diff --git a/plugin/builder-amazon-chroot/main.go b/plugin/builder-amazon-chroot/main.go index b7d71df44..13fa295f7 100644 --- a/plugin/builder-amazon-chroot/main.go +++ b/plugin/builder-amazon-chroot/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(chroot.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(chroot.Builder)) + server.Serve() } diff --git a/plugin/builder-amazon-ebs/main.go b/plugin/builder-amazon-ebs/main.go index f0598d749..ba6a6e380 100644 --- a/plugin/builder-amazon-ebs/main.go +++ b/plugin/builder-amazon-ebs/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(ebs.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(ebs.Builder)) + server.Serve() } diff --git a/plugin/builder-amazon-instance/main.go b/plugin/builder-amazon-instance/main.go index fc1c66b40..e87654107 100644 --- a/plugin/builder-amazon-instance/main.go +++ b/plugin/builder-amazon-instance/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(instance.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(instance.Builder)) + server.Serve() } diff --git a/plugin/builder-digitalocean/main.go b/plugin/builder-digitalocean/main.go index be8d706f0..a41ed3d65 100644 --- a/plugin/builder-digitalocean/main.go +++ b/plugin/builder-digitalocean/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(digitalocean.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(digitalocean.Builder)) + server.Serve() } diff --git a/plugin/builder-docker/main.go b/plugin/builder-docker/main.go index a5b69de0b..1ed06dd5e 100644 --- a/plugin/builder-docker/main.go +++ b/plugin/builder-docker/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(docker.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(docker.Builder)) + server.Serve() } diff --git a/plugin/builder-openstack/main.go b/plugin/builder-openstack/main.go index 076011003..7753e04ff 100644 --- a/plugin/builder-openstack/main.go +++ b/plugin/builder-openstack/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(openstack.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(openstack.Builder)) + server.Serve() } diff --git a/plugin/builder-qemu/main.go b/plugin/builder-qemu/main.go index 710742bad..b823ba501 100644 --- a/plugin/builder-qemu/main.go +++ b/plugin/builder-qemu/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(qemu.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(qemu.Builder)) + server.Serve() } diff --git a/plugin/builder-virtualbox/main.go b/plugin/builder-virtualbox/main.go index 1428c5b1d..136b2698e 100644 --- a/plugin/builder-virtualbox/main.go +++ b/plugin/builder-virtualbox/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(virtualbox.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(virtualbox.Builder)) + server.Serve() } diff --git a/plugin/builder-vmware/main.go b/plugin/builder-vmware/main.go index a449751b8..14a10cf5c 100644 --- a/plugin/builder-vmware/main.go +++ b/plugin/builder-vmware/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeBuilder(new(vmware.Builder)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterBuilder(new(vmware.Builder)) + server.Serve() } diff --git a/plugin/command-build/main.go b/plugin/command-build/main.go index e6a4397fd..1285e1e7a 100644 --- a/plugin/command-build/main.go +++ b/plugin/command-build/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeCommand(new(build.Command)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterCommand(new(build.Command)) + server.Serve() } diff --git a/plugin/command-fix/main.go b/plugin/command-fix/main.go index 8ae6d42a0..03677674d 100644 --- a/plugin/command-fix/main.go +++ b/plugin/command-fix/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeCommand(new(fix.Command)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterCommand(new(fix.Command)) + server.Serve() } diff --git a/plugin/command-inspect/main.go b/plugin/command-inspect/main.go index 6d2d7d3d5..9aeedc34c 100644 --- a/plugin/command-inspect/main.go +++ b/plugin/command-inspect/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeCommand(new(inspect.Command)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterCommand(new(inspect.Command)) + server.Serve() } diff --git a/plugin/command-validate/main.go b/plugin/command-validate/main.go index af093294e..1105ed75e 100644 --- a/plugin/command-validate/main.go +++ b/plugin/command-validate/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeCommand(new(validate.Command)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterCommand(new(validate.Command)) + server.Serve() } diff --git a/plugin/post-processor-vagrant/main.go b/plugin/post-processor-vagrant/main.go index 0224d26d1..cf84e3ff1 100644 --- a/plugin/post-processor-vagrant/main.go +++ b/plugin/post-processor-vagrant/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServePostProcessor(new(vagrant.PostProcessor)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterPostProcessor(new(vagrant.PostProcessor)) + server.Serve() } diff --git a/plugin/post-processor-vsphere/main.go b/plugin/post-processor-vsphere/main.go index 22317ab5d..10e7a8d40 100644 --- a/plugin/post-processor-vsphere/main.go +++ b/plugin/post-processor-vsphere/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServePostProcessor(new(vsphere.PostProcessor)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterPostProcessor(new(vsphere.PostProcessor)) + server.Serve() } diff --git a/plugin/provisioner-ansible-local/main.go b/plugin/provisioner-ansible-local/main.go index 0caf0427f..5477b1b55 100644 --- a/plugin/provisioner-ansible-local/main.go +++ b/plugin/provisioner-ansible-local/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeProvisioner(new(ansiblelocal.Provisioner)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterProvisioner(new(ansiblelocal.Provisioner)) + server.Serve() } diff --git a/plugin/provisioner-chef-solo/main.go b/plugin/provisioner-chef-solo/main.go index 4057f9925..e8a5bf11e 100644 --- a/plugin/provisioner-chef-solo/main.go +++ b/plugin/provisioner-chef-solo/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeProvisioner(new(chefsolo.Provisioner)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterProvisioner(new(chefsolo.Provisioner)) + server.Serve() } diff --git a/plugin/provisioner-file/main.go b/plugin/provisioner-file/main.go index 1f5a63413..1b746e050 100644 --- a/plugin/provisioner-file/main.go +++ b/plugin/provisioner-file/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeProvisioner(new(file.Provisioner)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterProvisioner(new(file.Provisioner)) + server.Serve() } diff --git a/plugin/provisioner-puppet-masterless/main.go b/plugin/provisioner-puppet-masterless/main.go index c510cbb78..45dba0a31 100644 --- a/plugin/provisioner-puppet-masterless/main.go +++ b/plugin/provisioner-puppet-masterless/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeProvisioner(new(puppetmasterless.Provisioner)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterProvisioner(new(puppetmasterless.Provisioner)) + server.Serve() } diff --git a/plugin/provisioner-salt-masterless/main.go b/plugin/provisioner-salt-masterless/main.go index 53ccd771a..584c4657e 100644 --- a/plugin/provisioner-salt-masterless/main.go +++ b/plugin/provisioner-salt-masterless/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeProvisioner(new(saltmasterless.Provisioner)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterProvisioner(new(saltmasterless.Provisioner)) + server.Serve() } diff --git a/plugin/provisioner-shell/main.go b/plugin/provisioner-shell/main.go index 07d18472e..03966b67b 100644 --- a/plugin/provisioner-shell/main.go +++ b/plugin/provisioner-shell/main.go @@ -6,5 +6,10 @@ import ( ) func main() { - plugin.ServeProvisioner(new(shell.Provisioner)) + server, err := plugin.Server() + if err != nil { + panic(err) + } + server.RegisterProvisioner(new(shell.Provisioner)) + server.Serve() } From 06d12773eb4b9fd9b1c042cf6fe1df9c3a04243f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 14:11:50 -0800 Subject: [PATCH 29/37] packer/rpc: improve logging for the MuxConn --- packer/rpc/muxconn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 87941b826..0663bc0c7 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -407,7 +407,7 @@ func (s *Stream) Write(p []byte) (int, error) { s.mu.Unlock() if state != streamStateEstablished { - return 0, fmt.Errorf("Stream in bad state to send: %d", state) + return 0, fmt.Errorf("Stream %d in bad state to send: %d", s.id, state) } return s.mux.write(s.id, muxPacketData, p) From 4c5d61709d4e46043e148a1a7fa1cd1246ceed03 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 14:12:00 -0800 Subject: [PATCH 30/37] packer/plugin: catch interrupts for every server --- packer/plugin/plugin.go | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/packer/plugin/plugin.go b/packer/plugin/plugin.go index 91aa91274..8012ffc4f 100644 --- a/packer/plugin/plugin.go +++ b/packer/plugin/plugin.go @@ -92,28 +92,19 @@ func Server() (*packrpc.Server, error) { return nil, err } + // Eat the interrupts + ch := make(chan os.Signal, 1) + signal.Notify(ch, os.Interrupt) + go func() { + var count int32 = 0 + for { + <-ch + newCount := atomic.AddInt32(&count, 1) + log.Printf("Received interrupt signal (count: %d). Ignoring.", newCount) + } + }() + // Serve a single connection log.Println("Serving a plugin connection...") return packrpc.NewServer(conn), nil } - -// Registers a signal handler to swallow and count interrupts so that the -// plugin isn't killed. The main host Packer process is responsible -// for killing the plugins when interrupted. -func countInterrupts() { - ch := make(chan os.Signal, 1) - signal.Notify(ch, os.Interrupt) - - go func() { - for { - <-ch - newCount := atomic.AddInt32(&Interrupts, 1) - log.Printf("Received interrupt signal (count: %d). Ignoring.", newCount) - } - }() -} - -// Tests whether or not the plugin was interrupted or not. -func Interrupted() bool { - return atomic.LoadInt32(&Interrupts) > 0 -} From 3a4150088839ac06356bbe4e855d94ae42c22580 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 15:12:16 -0800 Subject: [PATCH 31/37] packer/rpc: more robust communicator connection cleanup --- packer/rpc/communicator.go | 39 +++++++++++++++++++++++++++----------- packer/rpc/muxconn.go | 4 ++-- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/packer/rpc/communicator.go b/packer/rpc/communicator.go index de624a7af..3ea728bc1 100644 --- a/packer/rpc/communicator.go +++ b/packer/rpc/communicator.go @@ -95,6 +95,7 @@ func (c *communicator) Start(cmd *packer.RemoteCmd) (err error) { return } + log.Printf("[INFO] RPC client: Communicator ended with: %d", finished.ExitStatus) cmd.SetExited(finished.ExitStatus) }() @@ -146,17 +147,28 @@ func (c *communicator) Download(path string, w io.Writer) (err error) { return } -func (c *CommunicatorServer) Start(args *CommunicatorStartArgs, reply *interface{}) (err error) { +func (c *CommunicatorServer) Start(args *CommunicatorStartArgs, reply *interface{}) (error) { // Build the RemoteCmd on this side so that it all pipes over // to the remote side. var cmd packer.RemoteCmd cmd.Command = args.Command + // Create a channel to signal we're done so that we can close + // our stdin/stdout/stderr streams toClose := make([]io.Closer, 0) + doneCh := make(chan struct{}) + go func() { + <-doneCh + for _, conn := range toClose { + defer conn.Close() + } + }() + if args.StdinStreamId > 0 { conn, err := c.mux.Dial(args.StdinStreamId) if err != nil { - return err + close(doneCh) + return NewBasicError(err) } toClose = append(toClose, conn) @@ -166,7 +178,8 @@ func (c *CommunicatorServer) Start(args *CommunicatorStartArgs, reply *interface if args.StdoutStreamId > 0 { conn, err := c.mux.Dial(args.StdoutStreamId) if err != nil { - return err + close(doneCh) + return NewBasicError(err) } toClose = append(toClose, conn) @@ -176,38 +189,42 @@ func (c *CommunicatorServer) Start(args *CommunicatorStartArgs, reply *interface if args.StderrStreamId > 0 { conn, err := c.mux.Dial(args.StderrStreamId) if err != nil { - return err + close(doneCh) + return NewBasicError(err) } toClose = append(toClose, conn) cmd.Stderr = conn } + // Connect to the response address so we can write our result to it // when ready. responseC, err := c.mux.Dial(args.ResponseStreamId) if err != nil { - return err + close(doneCh) + return NewBasicError(err) } - responseWriter := gob.NewEncoder(responseC) // Start the actual command err = c.c.Start(&cmd) + if err != nil { + close(doneCh) + return NewBasicError(err) + } // Start a goroutine to spin and wait for the process to actual // exit. When it does, report it back to caller... go func() { + defer close(doneCh) defer responseC.Close() - for _, conn := range toClose { - defer conn.Close() - } - cmd.Wait() + log.Printf("[INFO] RPC endpoint: Communicator ended with: %d", cmd.ExitStatus) responseWriter.Encode(&CommandFinished{cmd.ExitStatus}) }() - return + return nil } func (c *CommunicatorServer) Upload(args *CommunicatorUploadArgs, reply *interface{}) (err error) { diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 0663bc0c7..e29d938bb 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -197,7 +197,7 @@ func (m *MuxConn) openStream(id uint32) (*Stream, error) { // Create the stream object and channel where data will be sent to dataR, dataW := io.Pipe() - writeCh := make(chan []byte, 10) + writeCh := make(chan []byte, 256) // Set the data channel so we can write to it. stream := &Stream{ @@ -315,7 +315,7 @@ func (m *MuxConn) loop() { select { case stream.writeCh <- data: default: - log.Printf("[ERR] Failed to write data, buffer full: %d", id) + panic(fmt.Sprintf("Failed to write data, buffer full for stream %d", id)) } } else { log.Printf("[ERR] Data received for stream in state: %d", stream.state) From e4dbad330d1612c23b4798d3f2cdbc8068e71a3a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 15:30:58 -0800 Subject: [PATCH 32/37] packer/rpc: rename uploadReader to uploadData because that makes sense --- packer/rpc/communicator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packer/rpc/communicator.go b/packer/rpc/communicator.go index 3ea728bc1..8f470075e 100644 --- a/packer/rpc/communicator.go +++ b/packer/rpc/communicator.go @@ -106,7 +106,7 @@ func (c *communicator) Start(cmd *packer.RemoteCmd) (err error) { func (c *communicator) Upload(path string, r io.Reader) (err error) { // Pipe the reader through to the connection streamId := c.mux.NextId() - go serveSingleCopy("uploadReader", c.mux, streamId, nil, r) + go serveSingleCopy("uploadData", c.mux, streamId, nil, r) args := CommunicatorUploadArgs{ Path: path, From 7372c32b6b57ca810c3e7ac8ea4a2e6905b36683 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 15:51:22 -0800 Subject: [PATCH 33/37] packer/rpc: implement proper close_wait state --- packer/rpc/muxconn.go | 38 +++++++++++++++++++------------------- packer/rpc/muxconn_test.go | 14 ++++++++------ 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index e29d938bb..63ac39139 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -150,6 +150,8 @@ func (m *MuxConn) Dial(id uint32) (io.ReadWriteCloser, error) { // and it closed all within the time period we wait above. // This case will be fixed when we have edge-triggered checks. fallthrough + case streamStateCloseWait: + fallthrough case streamStateEstablished: stream.mu.Unlock() return stream, nil @@ -274,7 +276,7 @@ func (m *MuxConn) loop() { case streamStateSynSent: stream.setState(streamStateEstablished) case streamStateFinWait1: - stream.remoteClose() + stream.setState(streamStateFinWait2) default: log.Printf("[ERR] Ack received for stream in state: %d", stream.state) } @@ -294,9 +296,15 @@ func (m *MuxConn) loop() { stream.mu.Lock() switch stream.state { case streamStateEstablished: + stream.setState(streamStateCloseWait) m.write(id, muxPacketAck, nil) - fallthrough + + // Close the writer on our end since we won't receive any + // more data. + stream.writeCh <- nil case streamStateFinWait1: + fallthrough + case streamStateFinWait2: stream.remoteClose() // Remove this stream from being active so that it @@ -364,34 +372,26 @@ const ( streamStateSynSent streamStateEstablished streamStateFinWait1 + streamStateFinWait2 + streamStateCloseWait ) func (s *Stream) Close() error { s.mu.Lock() - if s.state != streamStateEstablished { - s.mu.Unlock() + defer s.mu.Unlock() + + if s.state != streamStateEstablished && s.state != streamStateCloseWait { return fmt.Errorf("Stream in bad state: %d", s.state) } if _, err := s.mux.write(s.id, muxPacketFin, nil); err != nil { return err } - s.setState(streamStateFinWait1) - s.mu.Unlock() - for { - time.Sleep(50 * time.Millisecond) - s.mu.Lock() - switch s.state { - case streamStateFinWait1: - s.mu.Unlock() - case streamStateClosed: - s.mu.Unlock() - return nil - default: - defer s.mu.Unlock() - return fmt.Errorf("Stream %d went to bad state: %d", s.id, s.state) - } + if s.state == streamStateEstablished { + s.setState(streamStateFinWait1) + } else { + s.remoteClose() } return nil diff --git a/packer/rpc/muxconn_test.go b/packer/rpc/muxconn_test.go index e6c23f26f..392ee6b5a 100644 --- a/packer/rpc/muxconn_test.go +++ b/packer/rpc/muxconn_test.go @@ -118,18 +118,20 @@ func TestMuxConn_clientClosesStreams(t *testing.T) { client, server := testMux(t) defer client.Close() defer server.Close() - go server.Accept(0) + + go func() { + conn, err := server.Accept(0) + if err != nil { + t.Fatalf("err: %s", err) + } + conn.Close() + }() s0, err := client.Dial(0) if err != nil { t.Fatalf("err: %s", err) } - if err := client.Close(); err != nil { - t.Fatalf("err: %s", err) - } - - // This should block forever since we never write onto this stream. var data [1024]byte _, err = s0.Read(data[:]) if err != io.EOF { From 5dffab743986d20464efc0bfbec04c75d8075c66 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 16:23:47 -0800 Subject: [PATCH 34/37] packer/rpc: need a real lock for closing --- packer/rpc/muxconn.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 63ac39139..08c28bb25 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -48,8 +48,8 @@ func NewMuxConn(rwc io.ReadWriteCloser) *MuxConn { // Close closes the underlying io.ReadWriteCloser. This will also close // all streams that are open. func (m *MuxConn) Close() error { - m.mu.RLock() - defer m.mu.RUnlock() + m.mu.Lock() + defer m.mu.Unlock() // Close all the streams for _, w := range m.streams { From d9f79b0ecc76a0ea465da1c554ae7399c53c1113 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 16:49:14 -0800 Subject: [PATCH 35/37] packer/rpc: hard close all streams when underlying conn closes --- packer/rpc/muxconn.go | 13 ++++++++----- packer/rpc/muxconn_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 08c28bb25..34b5988c3 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -234,7 +234,13 @@ func (m *MuxConn) openStream(id uint32) (*Stream, error) { } func (m *MuxConn) loop() { - defer m.Close() + defer func() { + m.mu.Lock() + defer m.mu.Unlock() + for _, w := range m.streams { + w.remoteClose() + } + }() var id uint32 var packetType muxPacketType @@ -384,16 +390,13 @@ func (s *Stream) Close() error { return fmt.Errorf("Stream in bad state: %d", s.state) } - if _, err := s.mux.write(s.id, muxPacketFin, nil); err != nil { - return err - } - if s.state == streamStateEstablished { s.setState(streamStateFinWait1) } else { s.remoteClose() } + s.mux.write(s.id, muxPacketFin, nil) return nil } diff --git a/packer/rpc/muxconn_test.go b/packer/rpc/muxconn_test.go index 392ee6b5a..f4ba59db1 100644 --- a/packer/rpc/muxconn_test.go +++ b/packer/rpc/muxconn_test.go @@ -114,6 +114,32 @@ func TestMuxConn(t *testing.T) { <-doneCh } +func TestMuxConn_socketClose(t *testing.T) { + client, server := testMux(t) + defer client.Close() + defer server.Close() + + go func() { + _, err := server.Accept(0) + if err != nil { + t.Fatalf("err: %s", err) + } + + server.rwc.Close() + }() + + s0, err := client.Dial(0) + if err != nil { + t.Fatalf("err: %s", err) + } + + var data [1024]byte + _, err = s0.Read(data[:]) + if err != io.EOF { + t.Fatalf("err: %s", err) + } +} + func TestMuxConn_clientClosesStreams(t *testing.T) { client, server := testMux(t) defer client.Close() From f79daa0b1bd346522dada2727dd12eab1abac3b3 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 17:01:02 -0800 Subject: [PATCH 36/37] packer/rpc: edge-triggerd state changes for faster dial/accept --- packer/rpc/muxconn.go | 70 +++++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 34b5988c3..8ff8b4aec 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -84,23 +84,25 @@ func (m *MuxConn) Accept(id uint32) (io.ReadWriteCloser, error) { if stream.state != streamStateEstablished { // Go into the listening state stream.setState(streamStateListen) + + // Register a state change listener to wait for changes + stateCh := make(chan streamState, 10) + stream.registerStateListener(stateCh) + defer func() { + stream.mu.Lock() + defer stream.mu.Unlock() + stream.deregisterStateListener(stateCh) + }() + stream.mu.Unlock() // Wait for the connection to establish ACCEPT_ESTABLISH_LOOP: for { - time.Sleep(50 * time.Millisecond) - stream.mu.Lock() - switch stream.state { + state := <-stateCh + switch state { case streamStateListen: - stream.mu.Unlock() - case streamStateClosed: - // This can happen if it becomes established, some data is sent, - // and it closed all within the time period we wait above. - // This case will be fixed when we have edge-triggered checks. - fallthrough case streamStateEstablished: - stream.mu.Unlock() break ACCEPT_ESTABLISH_LOOP default: defer stream.mu.Unlock() @@ -137,23 +139,23 @@ func (m *MuxConn) Dial(id uint32) (io.ReadWriteCloser, error) { return nil, err } stream.setState(streamStateSynSent) + + // Register a state change listener to wait for changes + stateCh := make(chan streamState, 10) + stream.registerStateListener(stateCh) + defer func() { + stream.mu.Lock() + defer stream.mu.Unlock() + stream.deregisterStateListener(stateCh) + }() + stream.mu.Unlock() for { - time.Sleep(50 * time.Millisecond) - stream.mu.Lock() - switch stream.state { + state := <-stateCh + switch state { case streamStateSynSent: - stream.mu.Unlock() - case streamStateClosed: - // This can happen if it becomes established, some data is sent, - // and it closed all within the time period we wait above. - // This case will be fixed when we have edge-triggered checks. - fallthrough - case streamStateCloseWait: - fallthrough case streamStateEstablished: - stream.mu.Unlock() return stream, nil default: defer stream.mu.Unlock() @@ -203,10 +205,11 @@ func (m *MuxConn) openStream(id uint32) (*Stream, error) { // Set the data channel so we can write to it. stream := &Stream{ - id: id, - mux: m, - reader: dataR, - writeCh: writeCh, + id: id, + mux: m, + reader: dataR, + writeCh: writeCh, + stateChange: make(map[chan<- streamState]struct{}), } stream.setState(streamStateClosed) @@ -364,6 +367,7 @@ type Stream struct { mux *MuxConn reader io.Reader state streamState + stateChange map[chan<- streamState]struct{} stateUpdated time.Time mu sync.Mutex writeCh chan<- []byte @@ -421,7 +425,21 @@ func (s *Stream) remoteClose() { s.writeCh <- nil } +func (s *Stream) registerStateListener(ch chan<- streamState) { + s.stateChange[ch] = struct{}{} +} + +func (s *Stream) deregisterStateListener(ch chan<- streamState) { + delete(s.stateChange, ch) +} + func (s *Stream) setState(state streamState) { s.state = state s.stateUpdated = time.Now().UTC() + for ch, _ := range s.stateChange { + select { + case ch <- state: + default: + } + } } From 8a24c9b1777986b6124db55dda8a320ef552bc84 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 10 Dec 2013 17:09:17 -0800 Subject: [PATCH 37/37] packer/rpc: fix data race in MuxConn --- packer/rpc/muxconn.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packer/rpc/muxconn.go b/packer/rpc/muxconn.go index 8ff8b4aec..9201dfaa7 100644 --- a/packer/rpc/muxconn.go +++ b/packer/rpc/muxconn.go @@ -241,7 +241,9 @@ func (m *MuxConn) loop() { m.mu.Lock() defer m.mu.Unlock() for _, w := range m.streams { + w.mu.Lock() w.remoteClose() + w.mu.Unlock() } }()