diff --git a/benchkit/benchmark/benchmark_test.go b/benchkit/benchmark/benchmark_test.go index c4af8fbf..583fda11 100644 --- a/benchkit/benchmark/benchmark_test.go +++ b/benchkit/benchmark/benchmark_test.go @@ -257,11 +257,11 @@ func TestQuorumCallHonorsCallTimeout(t *testing.T) { registerReplyDroppingPeer(servers[0], 1<<30) // drops every reply for the test's duration go func() { _ = servers[0].ListenAndServe() }() - cfg, err := gorums.NewConfig(gorums.WithNodeList([]string{servers[0].Addr()}), gorumstest.InsecureDialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList([]string{servers[0].Addr()}), gorumstest.InsecureDialOptions(t)) if err != nil { t.Fatalf("NewConfig: %v", err) } - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) run := benchDescs[0].build(cfg, nil) // "QuorumCall" opts := benchkit.Options{ @@ -292,11 +292,11 @@ func TestRunAsyncQCBenchmarkCountsErrorsWithoutAborting(t *testing.T) { registerFailingQuorumCallPeer(servers[0], "quorum call failed") go func() { _ = servers[0].ListenAndServe() }() - cfg, err := gorums.NewConfig(gorums.WithNodeList([]string{servers[0].Addr()}), gorumstest.InsecureDialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList([]string{servers[0].Addr()}), gorumstest.InsecureDialOptions(t)) if err != nil { t.Fatalf("NewConfig: %v", err) } - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) opts := benchkit.Options{Workers: 1, Duration: 30 * time.Millisecond, MaxAsync: 10, QuorumSize: 1} result, err := runAsyncQCBenchmark(opts, cfg, diff --git a/benchkit/benchmark/target.go b/benchkit/benchmark/target.go index dfbdb916..a3b64a62 100644 --- a/benchkit/benchmark/target.go +++ b/benchkit/benchmark/target.go @@ -135,11 +135,11 @@ func setupCoordinator(opts *benchkit.Options, remotes []string, configSize int, opts.NumNodes = configSize } - cfg, err := gorums.NewConfig(gorums.WithNodeList(remotes[:opts.NumNodes]), dialOpts...) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(remotes[:opts.NumNodes]), dialOpts...) if err != nil { return target, nil, fmt.Errorf("configuration setup: %w", err) } target.Config = cfg - return target, func() { _ = cfg.Close() }, nil + return target, closeFn, nil } diff --git a/config.go b/config.go index f60af4d3..332ff2d4 100644 --- a/config.go +++ b/config.go @@ -61,14 +61,36 @@ type Config = conn.Config // Use [Config.Context] to create a ConfigContext from an existing context. type ConfigContext = conn.ConfigContext -// NewConfig returns a new [Config] based on the provided nodes and dial options. +// NewConfig returns a new [Config] based on the provided nodes and dial +// options, and a function that closes the configuration's connection pool. +// +// The returned configuration and every configuration derived from it, for +// example with [Config.Extend], [Config.Remove], or [Config.Sort], share one +// connection pool. Only the returned close function closes that pool, and it +// closes every node in it, including nodes that [Config.Extend] added later. +// After the close function returns: +// - calls to the pool's nodes fail with an Unavailable "node closed" error; +// - [Config.Extend] on any configuration in the pool returns an error. +// +// One exception: a node that runs in-process, or that reuses a server's +// inbound stream, owns no connection, so calls to it still work. +// +// The close function is idempotent and safe for concurrent use; every call +// returns after the nodes are closed. It returns no error, because closing a +// connection has no failure that a caller can act on. +// +// On error, NewConfig returns a nil configuration and a nil close function. // // Example: // -// cfg, err := NewConfig( +// cfg, closeFn, err := NewConfig( // gorums.WithNodeList([]string{"localhost:8080", "localhost:8081", "localhost:8082"}), // gorums.WithGRPCDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), // ) -func NewConfig(nodes NodeSource, opts ...DialOption) (Config, error) { +// if err != nil { +// return err +// } +// defer closeFn() +func NewConfig(nodes NodeSource, opts ...DialOption) (Config, func(), error) { return conn.NewConfig(nodes, opts...) } diff --git a/config_test.go b/config_test.go index bb7eb769..be0a9b8b 100644 --- a/config_test.go +++ b/config_test.go @@ -97,9 +97,9 @@ func TestNewConfig(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfg, err := gorums.NewConfig(tt.nodes, gorumstest.InsecureDialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(tt.nodes, gorumstest.InsecureDialOptions(t)) if err == nil { - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) } if tt.wantErr != "" { if err == nil { @@ -108,6 +108,9 @@ func TestNewConfig(t *testing.T) { if err.Error() != tt.wantErr { t.Errorf("Error = %q, want %q", err.Error(), tt.wantErr) } + if cfg != nil || closeFn != nil { + t.Errorf("NewConfig() on error = %v, %p; want nil, nil", cfg.NodeIDs(), closeFn) + } return } if err != nil { @@ -154,13 +157,13 @@ func TestNewConfigWithBackChannel(t *testing.T) { if srv != nil { t.Cleanup(srv.Stop) } - cfg, err := gorums.NewConfig( + cfg, closeFn, err := gorums.NewConfig( gorums.WithNodeList(nodeList), gorums.WithBackChannel(srv), gorumstest.InsecureDialOptions(t), ) if err == nil { - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) } if tt.wantErr != "" { if err == nil { @@ -169,6 +172,9 @@ func TestNewConfigWithBackChannel(t *testing.T) { if err.Error() != tt.wantErr { t.Errorf("Error = %q, want %q", err.Error(), tt.wantErr) } + if cfg != nil || closeFn != nil { + t.Errorf("NewConfig() on error = %v, %p; want nil, nil", cfg.NodeIDs(), closeFn) + } return } if err != nil { @@ -237,12 +243,6 @@ func TestEmptyConfiguration(t *testing.T) { } }) - t.Run("CloseNil", func(t *testing.T) { - if err := empty.Close(); err != nil { - t.Fatalf("empty.Close() error = %v, want nil", err) - } - }) - t.Run("ContainsFalse", func(t *testing.T) { if empty.Contains(1) { t.Fatal("empty.Contains(1) = true, want false") @@ -379,11 +379,11 @@ func TestConfigExtend(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - c, err := gorums.NewConfig(tt.initialNodes, gorumstest.InsecureDialOptions(t)) + c, closeFn, err := gorums.NewConfig(tt.initialNodes, gorumstest.InsecureDialOptions(t)) if err != nil { t.Fatal(err) } - t.Cleanup(gorumstest.Closer(t, c)) + t.Cleanup(closeFn) c2, err := c.Extend(tt.extendNodes) if tt.wantErr != "" { @@ -409,11 +409,11 @@ func TestConfigExtendConcurrent(t *testing.T) { addrs := gorumstest.Servers(t, 6, func(_ int) gorumstest.ServerIface { return gorums.NewServer() }) // Create base configuration so that concurrent Extend operations share the same node registry. - cfg, err := gorums.NewConfig(gorums.WithNodeList(addrs[0:1]), gorumstest.DialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs[0:1]), gorumstest.DialOptions(t)) if err != nil { t.Fatal(err) } - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) // Create multiple node maps to extend with, each containing a unique new node. // These maps will be used concurrently to verify that Extend can safely mutate @@ -448,49 +448,64 @@ func TestConfigExtendConcurrent(t *testing.T) { } } -// TestConfigClose verifies that Close on any configuration derived from one -// NewConfig call closes every node in that pool, is idempotent, and leaves no -// live node behind when Extend runs after or concurrently with Close. +// TestConfigClose verifies that the close function returned by NewConfig +// closes every node in the pool, including nodes that Extend added, that it is +// idempotent and safe for concurrent use, and that it leaves no live node +// behind when Extend runs after or concurrently with it. func TestConfigClose(t *testing.T) { - t.Run("SubConfigClosesPool", func(t *testing.T) { - cfg := gorumstest.Config(t, 3, nil) - outside := gorumstest.PeerNode(t, cfg, 3) - call := func() error { - ctx := gorumstest.Context(t, 5*time.Second) - _, err := gorumsimpl.RemoteCall[*pb.StringValue, *pb.StringValue](outside.Context(ctx), pb.String("x"), mock.TestMethod) - return err - } - if err := call(); err != nil { - t.Fatalf("call before Close: %v", err) - } - - sub := cfg.Remove(outside.ID()) - if err := sub.Close(); err != nil { - t.Fatalf("sub.Close() = %v, want nil", err) - } - err := call() + call := func(t *testing.T, node *gorums.Node) error { + t.Helper() + ctx := gorumstest.Context(t, 5*time.Second) + _, err := gorumsimpl.RemoteCall[*pb.StringValue, *pb.StringValue](node.Context(ctx), pb.String("x"), mock.TestMethod) + return err + } + wantClosed := func(t *testing.T, node *gorums.Node) { + t.Helper() + err := call(t, node) if status.Code(err) != codes.Unavailable || !strings.Contains(err.Error(), "node closed") { - t.Errorf("call on node %d outside the closed sub-configuration: err = %v, want Unavailable node closed", outside.ID(), err) + t.Errorf("call on node %d after close: err = %v, want Unavailable node closed", node.ID(), err) } - if err := sub.Close(); err != nil { - t.Errorf("second sub.Close() = %v, want nil", err) + } + + t.Run("ClosesExtendedNodes", func(t *testing.T) { + addrs := gorumstest.Servers(t, 2, nil) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs[:1]), gorumstest.DialOptions(t)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(closeFn) + ext, err := cfg.Extend(gorums.WithNodeList(addrs[1:])) + if err != nil { + t.Fatal(err) + } + for _, node := range ext { + if err := call(t, node); err != nil { + t.Fatalf("call on node %d before close: %v", node.ID(), err) + } } - if err := cfg.Close(); err != nil { - t.Errorf("cfg.Close() after sub.Close() = %v, want nil", err) + closeFn() + for _, node := range ext { + wantClosed(t, node) } }) - t.Run("EmptySubConfigKeepsPool", func(t *testing.T) { - cfg := gorumstest.Config(t, 2, nil) - node := gorumstest.PeerNode(t, cfg, 1) - empty := cfg.Remove(cfg.NodeIDs()...) - if err := empty.Close(); err != nil { - t.Fatalf("empty.Close() = %v, want nil", err) + t.Run("Idempotent", func(t *testing.T) { + addrs := gorumstest.Servers(t, 1, nil) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs), gorumstest.DialOptions(t)) + if err != nil { + t.Fatal(err) } - ctx := gorumstest.Context(t, 5*time.Second) - if _, err := gorumsimpl.RemoteCall[*pb.StringValue, *pb.StringValue](node.Context(ctx), pb.String("x"), mock.TestMethod); err != nil { - t.Errorf("call after closing an empty sub-configuration: %v, want nil", err) + t.Cleanup(closeFn) + node := cfg.Nodes()[0] + var wg sync.WaitGroup + for range 3 { + wg.Go(func() { + closeFn() + // Every call returns only after the nodes are closed. + wantClosed(t, node) + }) } + wg.Wait() }) t.Run("ExtendAfterClose", func(t *testing.T) { @@ -505,42 +520,48 @@ func TestConfigClose(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfg, err := gorums.NewConfig(gorums.WithNodeList(addrs[:1]), gorumstest.DialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs[:1]), gorumstest.DialOptions(t)) if err != nil { t.Fatal(err) } - t.Cleanup(gorumstest.Closer(t, cfg)) - if err := cfg.Close(); err != nil { - t.Fatalf("cfg.Close() = %v, want nil", err) - } + closeFn() ext, err := cfg.Extend(tt.nodes) if err == nil { - t.Cleanup(gorumstest.Closer(t, ext)) - t.Fatalf("cfg.Extend() after Close = %v, nil; want error", ext.NodeIDs()) + t.Fatalf("cfg.Extend() after close = %v, nil; want error", ext.NodeIDs()) } if ext != nil { - t.Errorf("cfg.Extend() after Close = %v, want nil configuration", ext.NodeIDs()) + t.Errorf("cfg.Extend() after close = %v, want nil configuration", ext.NodeIDs()) } }) } }) + t.Run("NewConfigError", func(t *testing.T) { + addrs := gorumstest.Servers(t, 1, nil) + // NewConfig builds node 1 before it rejects the duplicate address, so + // it must close node 1 itself; goleak checks that at cleanup. + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList([]string{addrs[0], addrs[0]}), gorumstest.DialOptions(t)) + if err == nil { + closeFn() + t.Fatalf("NewConfig() with a duplicate address = %v, nil; want error", cfg.NodeIDs()) + } + if cfg != nil || closeFn != nil { + t.Errorf("NewConfig() on error = %v, %p; want nil, nil", cfg.NodeIDs(), closeFn) + } + }) + t.Run("ConcurrentExtend", func(t *testing.T) { addrs := gorumstest.Servers(t, 2, nil) for range 10 { - cfg, err := gorums.NewConfig(gorums.WithNodeList(addrs[:1]), gorumstest.DialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs[:1]), gorumstest.DialOptions(t)) if err != nil { t.Fatal(err) } var wg sync.WaitGroup + wg.Go(closeFn) wg.Go(func() { - if err := cfg.Close(); err != nil { - t.Errorf("cfg.Close() = %v, want nil", err) - } - }) - wg.Go(func() { - // Extend either joins the pool before Close takes its snapshot + // Extend either joins the pool before close takes its snapshot // and is closed with it, or it fails. Neither outcome may leave // a live node behind; goleak checks that at cleanup. _, _ = cfg.Extend(gorums.WithNodeList(addrs[1:])) @@ -713,11 +734,11 @@ func TestConfigImmutability(t *testing.T) { } func TestConfigWithoutErrors(t *testing.T) { - cfg, err := gorums.NewConfig(gorums.WithNodes(nodeMap), gorumstest.InsecureDialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodes(nodeMap), gorumstest.InsecureDialOptions(t)) if err != nil { t.Fatal(err) } - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) timeoutErr := errors.New("timeout") connRefusedErr := errors.New("connection refused") diff --git a/doc/dev-guide.md b/doc/dev-guide.md index 3871d4f3..65d1326d 100644 --- a/doc/dev-guide.md +++ b/doc/dev-guide.md @@ -240,7 +240,8 @@ classDiagram A `Config` is a slice of `*Node`, and each `Node` has a fixed `stream.Transport`. An `outboundManager` creates and owns the nodes of the configurations built by `NewConfig` and `Config.Extend`. -`Config.Close` on any of these configurations closes the manager and all its nodes. +`NewConfig` returns the manager's `Close` method as the close function; `Config` has no `Close` method, so only the creator of a pool can close it. +The manager's `Close` closes all its nodes and logs any node's close error, since a caller cannot act on it. `Config.Extend` fails on a closed manager before it builds any node. A closed manager also rejects new nodes under the same lock that guards its node list, so a node that races with `Close` cannot outlive it. A server's `InboundManager` owns the nodes for its known peers and for the peer-capable clients that connect to it, and maintains the connected-peer and connected-client configurations. diff --git a/doc/user-guide.md b/doc/user-guide.md index b27ee955..4358dae7 100644 --- a/doc/user-guide.md +++ b/doc/user-guide.md @@ -348,7 +348,7 @@ go gorumsSrv.ListenAndServe() Next, we write client code to call RPCs on our servers. The first thing we need to do is to create a `Config` using `gorums.NewConfig`. `NewConfig` establishes connections to the given nodes and returns a configuration -ready for making RPC calls. +ready for making RPC calls, and a function that closes those connections. We can forward gRPC dial options to `NewConfig` if needed. Below we use only a simple insecure connection option. @@ -371,7 +371,7 @@ func ExampleStorageClient() { "127.0.0.1:8082", } // Create a configuration including all nodes - allNodesConfig, err := gorums.NewConfig( + allNodesConfig, closeFn, err := gorums.NewConfig( gorums.WithNodeList(addrs), gorums.WithGRPCDialOptions( grpc.WithTransportCredentials(insecure.NewCredentials()), @@ -380,7 +380,7 @@ func ExampleStorageClient() { if err != nil { log.Fatalln("error creating read config:", err) } - defer allNodesConfig.Close() + defer closeFn() ``` A configuration is a set of nodes on which RPC calls can be invoked. @@ -388,7 +388,7 @@ A configuration is a set of nodes on which RPC calls can be invoked. The `Config` type has several useful methods for combining and filtering configurations. Inspect the package documentation or source code for details. -Configurations derived from `allNodesConfig` share its connection pool, and `Close` on any of them closes the whole pool; see [Closing Configurations](#closing-configurations). +Configurations derived from `allNodesConfig` share its connection pool, and only `closeFn` closes it; see [Closing Configurations](#closing-configurations). We can now invoke the WriteUnicast RPC on each `node` in the configuration: @@ -702,7 +702,7 @@ func ExampleStorageClient() { } // Create a configuration with all nodes - config, err := gorums.NewConfig( + config, closeFn, err := gorums.NewConfig( gorums.WithNodeList(addrs), gorums.WithGRPCDialOptions( grpc.WithTransportCredentials(insecure.NewCredentials()), @@ -711,7 +711,7 @@ func ExampleStorageClient() { if err != nil { log.Fatalln("error creating configuration:", err) } - defer config.Close() + defer closeFn() ctx := context.Background() cfgCtx := config.Context(ctx) @@ -1216,11 +1216,15 @@ gorumsSrv := gorums.NewServer( The connecting client attaches the metadata with `WithMetadata`: ```go -config, err := gorums.NewConfig( +config, closeFn, err := gorums.NewConfig( gorums.WithNodeList(addrs), gorums.WithMetadata(metadata.New(map[string]string{"client-id": "replica-3"})), gorums.WithGRPCDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), ) +if err != nil { + return err +} +defer closeFn() ``` ### WithPeerChange @@ -1475,7 +1479,7 @@ func ExampleConfigClient() { "127.0.0.1:8082", } // Create base configuration c1 from addrs, giving |c1| = 3. - c1, err := gorums.NewConfig( + c1, closeFn, err := gorums.NewConfig( gorums.WithNodeList(addrs), gorums.WithGRPCDialOptions( grpc.WithTransportCredentials(insecure.NewCredentials()), @@ -1484,7 +1488,7 @@ func ExampleConfigClient() { if err != nil { log.Fatalln("error creating configuration:", err) } - defer c1.Close() + defer closeFn() newAddrs := []string{ "127.0.0.1:9080", @@ -1525,17 +1529,19 @@ func ExampleConfigClient() { ### Closing Configurations All configurations derived from one `NewConfig` call share one connection pool. -In the example above, `c1` through `c8` share the pool that `NewConfig` created. -`Close` on any non-empty one of them closes the whole pool, including nodes that are not in that configuration. -An empty configuration, such as `c1.Remove(c1.NodeIDs()...)`, belongs to no pool, so `Close` on it does nothing. -For example, `c5.Close()` also closes the first node of `c1`. -Thus, call `Close` once, on the configuration that `NewConfig` returned, when the application no longer needs any configuration in the pool. - -After `Close`, calls to the pool's nodes fail with an `Unavailable` "node closed" error. -There is one exception: a node that runs in-process, or that reuses a server's inbound stream, owns no connection, so `Close` leaves it usable. -`Extend` returns an error on any configuration in a closed pool. -`Close` is idempotent and safe for concurrent use; a second call returns `nil`. -To stop using some nodes without closing the others, derive a smaller configuration with `Remove` or `Difference` and do not call `Close` on it. +In the example above, `c1` through `c8` share the pool that `NewConfig` created, and `c2` added two nodes to it. +Only the close function that `NewConfig` returned closes the pool. +A derived configuration is a view of the pool and has no `Close` method, so closing one view cannot affect another. +To stop using some nodes, derive a smaller configuration with `Remove` or `Difference`. + +The close function closes every node in the pool, including the nodes that `Extend` added. +After the close function returns, calls to the pool's nodes fail with an `Unavailable` "node closed" error. +`Extend` on any configuration in the pool also returns an error. +There is one exception: a node that runs in-process, or that reuses a server's inbound stream, owns no connection, so it stays usable. +The close function is idempotent and safe for concurrent use. +It returns no error, because closing a connection has no failure that a caller can act on. + +A `Server` configured with `WithPeers` owns its peer configuration, and `Stop` and `GracefulStop` close it. ## Latency-Based Node Selection @@ -1982,11 +1988,15 @@ clientSrv.RegisterHandler(pb.MyMethod, myHandler) // The configuration is used to reach the server(s); WithBackChannel installs // clientSrv as the back-channel handler on its connections. -config, err := gorums.NewConfig( +config, closeFn, err := gorums.NewConfig( gorums.WithNodeList(serverAddrs), gorums.WithBackChannel(clientSrv), gorums.WithGRPCDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), ) +if err != nil { + return err +} +defer closeFn() ``` When the remote server dispatches a back-channel call via `ctx.ConnectedClients()`, the call arrives on the same gRPC stream the client opened and is routed to `clientSrv` for dispatch. diff --git a/examples/storage/client.go b/examples/storage/client.go index 8f7ab082..d8517d94 100644 --- a/examples/storage/client.go +++ b/examples/storage/client.go @@ -13,14 +13,14 @@ func runClient(addresses []string) error { if len(addresses) < 1 { return fmt.Errorf("no server addresses provided") } - cfg, err := gorums.NewConfig( + cfg, closeFn, err := gorums.NewConfig( gorums.WithNodeList(addresses), gorums.WithGRPCDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), ) if err != nil { return err } - defer cfg.Close() + defer closeFn() return runRepl(cfg) } diff --git a/gorumstest/gorumstest.go b/gorumstest/gorumstest.go index 267e4bda..a9c050b1 100644 --- a/gorumstest/gorumstest.go +++ b/gorumstest/gorumstest.go @@ -9,7 +9,6 @@ package gorumstest import ( "context" - "io" "testing" "time" @@ -143,11 +142,11 @@ func Config(t testing.TB, numServers int, srvFn func(i int) ServerIface, opts .. // Create configuration and register its cleanup LAST so it runs FIRST (LIFO) dialOptions := append([]gorums.DialOption{DialOptions(t)}, testOpts.dialOpts...) - cfg, err := gorums.NewConfig(testOpts.nodeSource(addrs), dialOptions...) + cfg, closeFn, err := gorums.NewConfig(testOpts.nodeSource(addrs), dialOptions...) if err != nil { t.Fatal(err) } - t.Cleanup(Closer(t, cfg)) + t.Cleanup(closeFn) return cfg } @@ -163,11 +162,11 @@ func UnreachableConfig(t testing.TB, addrs ...string) gorums.Config { if len(addrs) == 0 { addrs = []string{unreachableSentinelAddr} } - cfg, err := gorums.NewConfig(gorums.WithNodeList(addrs), InsecureDialOptions(t)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs), InsecureDialOptions(t)) if err != nil { t.Fatalf("NewConfig: %v", err) } - t.Cleanup(Closer(t, cfg)) + t.Cleanup(closeFn) return cfg } @@ -197,13 +196,3 @@ func PeerNode(t testing.TB, cfg gorums.Config, id uint32) *gorums.Node { t.Fatalf("node %d not in config %v", id, cfg.NodeIDs()) return nil } - -// Closer returns a cleanup function that closes c. -func Closer(t testing.TB, c io.Closer) func() { - t.Helper() - return func() { - if err := c.Close(); err != nil { - t.Errorf("c.Close() = %q, expected no error", err.Error()) - } - } -} diff --git a/gorumstest/servers.go b/gorumstest/servers.go index ba701e17..d44a8e7b 100644 --- a/gorumstest/servers.go +++ b/gorumstest/servers.go @@ -31,8 +31,8 @@ type ServerIface = servers.ServerIface // Example usage: // // addrs := gorumstest.Servers(t, 3, serverFn) -// cfg, err := gorums.NewConfig(gorums.WithNodeList(addrs), gorumstest.DialOptions(t)) -// t.Cleanup(gorumstest.Closer(t, cfg)) +// cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs), gorumstest.DialOptions(t)) +// t.Cleanup(closeFn) // ... // // This function can be used by other packages for testing purposes, as long as diff --git a/internal/conn/config.go b/internal/conn/config.go index e0b0d092..97d1c49d 100644 --- a/internal/conn/config.go +++ b/internal/conn/config.go @@ -33,7 +33,8 @@ func (c ConfigContext) Config() Config { // // Example: // -// config, _ := gorums.NewConfig(gorums.WithNodeList(addrs), dialOpts...) +// config, closeFn, _ := gorums.NewConfig(gorums.WithNodeList(addrs), dialOpts...) +// defer closeFn() // cfgCtx := config.Context(context.Background()) // resp, err := paxos.Prepare(cfgCtx, req).Majority() func (c Config) Context(parent context.Context) *ConfigContext { @@ -43,35 +44,57 @@ func (c Config) Context(parent context.Context) *ConfigContext { return &ConfigContext{Context: parent, cfg: c} } -// NewConfig returns a new [Config] based on the provided nodes and dial options. +// NewConfig returns a new [Config] based on the provided nodes and dial +// options, and a function that closes the configuration's connection pool. +// +// The returned configuration and every configuration derived from it, for +// example with [Config.Extend], [Config.Remove], or [Config.Sort], share one +// connection pool. Only the returned close function closes that pool, and it +// closes every node in it, including nodes that [Config.Extend] added later. +// After the close function returns: +// - calls to the pool's nodes fail with an Unavailable "node closed" error; +// - [Config.Extend] on any configuration in the pool returns an error. +// +// One exception: a node that runs in-process, or that reuses a server's +// inbound stream, owns no connection, so calls to it still work. +// +// The close function is idempotent and safe for concurrent use; every call +// returns after the nodes are closed. It returns no error, because closing a +// connection has no failure that a caller can act on. +// +// On error, NewConfig returns a nil configuration and a nil close function. // // Example: // -// cfg, err := NewConfig( +// cfg, closeFn, err := NewConfig( // gorums.WithNodeList([]string{"localhost:8080", "localhost:8081", "localhost:8082"}), // gorums.WithGRPCDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), // ) -func NewConfig(nodes NodeSource, opts ...DialOption) (Config, error) { +// if err != nil { +// return err +// } +// defer closeFn() +func NewConfig(nodes NodeSource, opts ...DialOption) (Config, func(), error) { if nodes == nil { - return nil, fmt.Errorf("gorums: missing required node list") + return nil, nil, fmt.Errorf("gorums: missing required node list") } mgr := newOutboundManager(opts...) if err := mgr.opts.Err; err != nil { - _ = mgr.Close() - return nil, err + mgr.Close() + return nil, nil, err } cfg, err := nodes.newConfig(mgr) if err != nil { - _ = mgr.Close() - return nil, err + mgr.Close() + return nil, nil, err } - return cfg, nil + return cfg, mgr.Close, nil } // Extend returns a new Config combining c with new nodes from the provided NodeSource. -// The new nodes join c's connection pool, so [Config.Close] on any -// configuration in that pool also closes them. A node whose ID and address -// are already in the pool is reused, not dialed again. +// The new nodes join c's connection pool, so the close function that +// [NewConfig] returned for that pool also closes them. A node whose ID and +// address are already in the pool is reused, not dialed again. // Extend returns an error if c is empty or if c's connection pool is closed. func (c Config) Extend(nodes NodeSource) (Config, error) { if len(c) == 0 { @@ -123,26 +146,6 @@ func (c Config) Equal(b Config) bool { return true } -// Close closes the connection pool that c belongs to. -// -// All non-empty configurations derived from one [NewConfig] call share one -// pool, whether they come from [Config.Extend], [Config.Add], [Config.Remove], -// or another method. Close on any of them closes the whole pool, including -// nodes that are not in c. After Close, calls to the pool's nodes fail with an -// Unavailable "node closed" error, and [Config.Extend] returns an error. -// A node that runs in-process or reuses a server's inbound stream owns no -// connection, so Close leaves it usable. -// -// Close is idempotent and safe for concurrent use. A second call returns nil. -// An empty configuration belongs to no pool, so Close on it does nothing and -// returns nil. -func (c Config) Close() error { - if mgr := c.mgr(); mgr != nil { - return mgr.Close() - } - return nil -} - // Contains reports whether c contains a node with the given ID. func (c Config) Contains(id uint32) bool { return slices.ContainsFunc(c, func(n *Node) bool { return n.id == id }) diff --git a/internal/conn/config_test.go b/internal/conn/config_test.go index e06b9f68..e84026a2 100644 --- a/internal/conn/config_test.go +++ b/internal/conn/config_test.go @@ -222,26 +222,3 @@ func TestConfigSort(t *testing.T) { } }) } - -// TestConfigCloseWithoutManager verifies that Close returns nil for a -// configuration that has no outbound manager, such as an empty configuration -// or one whose nodes a server's inbound manager created. -func TestConfigCloseWithoutManager(t *testing.T) { - tests := []struct { - name string - cfg Config - }{ - {name: "Nil", cfg: nil}, - {name: "Empty", cfg: Config{}}, - {name: "NoManager", cfg: Config{newTestNode(1, nil), newTestNode(2, nil)}}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - for range 2 { - if err := tt.cfg.Close(); err != nil { - t.Errorf("Close() = %v, want nil", err) - } - } - }) - } -} diff --git a/internal/conn/outbound_manager.go b/internal/conn/outbound_manager.go index 8f60dfa0..91b4e74e 100644 --- a/internal/conn/outbound_manager.go +++ b/internal/conn/outbound_manager.go @@ -53,9 +53,9 @@ var errConfigClosed = errors.New("gorums: configuration is closed") // Close closes all node connections and any client streams. Once Close // starts, the manager rejects new nodes. Close is idempotent and safe for -// concurrent use; every call returns after the nodes are closed. -func (m *outboundManager) Close() error { - var err error +// concurrent use; every call returns after the nodes are closed. It logs a +// node's close error with the manager's logger, if one is set. +func (m *outboundManager) Close() { m.closeOnce.Do(func() { m.mu.Lock() m.closed = true @@ -64,10 +64,14 @@ func (m *outboundManager) Close() error { // Close the nodes outside mu: closing a node waits for its // goroutines, and the closed flag stops addNode from adding more. for _, node := range nodes { - err = errors.Join(err, node.close()) + // A close error is unlikely and needs no action: gRPC reports one + // only for a connection that is already closed, and each + // connection is closed once. The log is for diagnosis only. + if err := node.close(); err != nil && m.logger != nil { + m.logger.Printf("node %d: close: %v", node.id, err) + } } }) - return err } // Node returns the node with the given identifier if present. diff --git a/server.go b/server.go index 81daf0b3..7d1b2733 100644 --- a/server.go +++ b/server.go @@ -38,6 +38,7 @@ type Server struct { lis net.Listener // active listener; set by Serve, ListenAndServe, or NewLocalServers listenAddr string // address recorded by WithAddr outbound Config // outbound config; nil if WithPeers was not used + closePeers func() // closes outbound's pool; nil if WithPeers was not used } // NodeID returns this server's own [Config] node ID, as configured with @@ -140,21 +141,23 @@ func newServer(opts ...ServerOption) (*Server, error) { s.srv = stream.NewServer(serverOpts.connectCallback, s.im) stream.RegisterGorumsServer(s.grpcServer, s.srv) if serverOpts.peerNodes != nil { - cfg, err := s.newPeerConfig(serverOpts.peerNodes, serverOpts.outboundDialOpts) + cfg, closePeers, err := s.newPeerConfig(serverOpts.peerNodes, serverOpts.outboundDialOpts) if err != nil { s.grpcServer.Stop() return nil, err } s.outbound = cfg + s.closePeers = closePeers s.im.SetPeerConfig(cfg) } return s, nil } // newPeerConfig builds the outbound [Config] this server uses to call other -// servers. It installs the server as the back-channel request handler so the -// remote can dispatch requests back over the same connection. -func (s *Server) newPeerConfig(nodes NodeSource, dialOpts []DialOption) (Config, error) { +// servers, and the function that closes its connection pool. It installs the +// server as the back-channel request handler so the remote can dispatch +// requests back over the same connection. +func (s *Server) newPeerConfig(nodes NodeSource, dialOpts []DialOption) (Config, func(), error) { opts := append([]DialOption{withServer(s)}, dialOpts...) return NewConfig(nodes, opts...) } @@ -329,8 +332,8 @@ func (s *Server) WaitForAll(ctx context.Context) (Config, error) { func (s *Server) GracefulStop() { s.grpcServer.GracefulStop() s.im.Close() - if s.outbound != nil { - _ = s.outbound.Close() + if s.closePeers != nil { + s.closePeers() } } @@ -351,8 +354,8 @@ func (s *Server) Stop() { if lis != nil { _ = lis.Close() } - if s.outbound != nil { - _ = s.outbound.Close() + if s.closePeers != nil { + s.closePeers() } } diff --git a/server_internal_test.go b/server_internal_test.go index 07e926ba..81d2da96 100644 --- a/server_internal_test.go +++ b/server_internal_test.go @@ -2,7 +2,6 @@ package gorums import ( "context" - "io" "slices" "strings" "sync" @@ -30,9 +29,8 @@ import ( // (building package gorums's tests would require gorumstest, which requires // gorums). The helpers below build the server and dial-option setup this file // needs directly on top of the gorums-independent internal/testutils/servers -// package instead of gorumstest.Servers, gorumstest.DialOptions, -// gorumstest.Closer, and gorumstest.Context, so this file has no dependency on -// gorumstest. +// package instead of gorumstest.Servers, gorumstest.DialOptions, and +// gorumstest.Context, so this file has no dependency on gorumstest. // testStartServers starts numServers servers via srvFn and stops them, and // verifies no goroutines were leaked, when the test finishes. @@ -52,16 +50,6 @@ func testDialOptions(t testing.TB) DialOption { return WithGRPCDialOptions(servers.DialOptions(t)...) } -// testCloser returns a cleanup function that closes the given io.Closer. -func testCloser(t testing.TB, c io.Closer) func() { - t.Helper() - return func() { - if err := c.Close(); err != nil { - t.Errorf("c.Close() = %q, expected no error", err.Error()) - } - } -} - // testTimeoutContext creates a context with timeout, using t.Context() as the // parent, that automatically cancels on cleanup. func testTimeoutContext(t testing.TB, timeout time.Duration) context.Context { @@ -132,18 +120,19 @@ func peerNodes() NodeSource { } // connectAsPeer creates a Config that identifies itself as peerID by sending -// gorums-node-id metadata, connects to addrs, and returns the configuration. -// Config cleanup is registered via t.Cleanup; callers may also close it -// explicitly (e.g., to test disconnect) — Close is idempotent. -func connectAsPeer(t *testing.T, peerID uint32, addrs []string) Config { +// gorums-node-id metadata, connects to addrs, and returns the configuration +// and its close function. The close function is registered via t.Cleanup; +// callers may also call it explicitly (e.g., to test disconnect), since it is +// idempotent. +func connectAsPeer(t *testing.T, peerID uint32, addrs []string) (Config, func()) { t.Helper() peerMD := conn.MetadataWithNodeID(peerID) - cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithMetadata(peerMD)) + cfg, closeFn, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithMetadata(peerMD)) if err != nil { t.Fatalf("NewConfig() error: %v", err) } - t.Cleanup(testCloser(t, cfg)) - return cfg + t.Cleanup(closeFn) + return cfg, closeFn } // TestConfigExtendUsesKnownDedupPeer verifies that extending a dedup @@ -179,11 +168,11 @@ func TestConfigExtendUsesKnownDedupPeer(t *testing.T) { t.Cleanup(cleanup) } - initial, err := NewConfig(WithNodes(map[uint32]mock.NodeAddr{2: peers[2]}), insecureDialOpts, withServer(srv), conn.WithStreamDedup()) + initial, closeFn, err := NewConfig(WithNodes(map[uint32]mock.NodeAddr{2: peers[2]}), insecureDialOpts, withServer(srv), conn.WithStreamDedup()) if err != nil { t.Fatalf("initial configuration: %v", err) } - t.Cleanup(testCloser(t, initial)) + t.Cleanup(closeFn) extended, err := initial.Extend(WithNodes(map[uint32]mock.NodeAddr{1: peers[1]})) if err != nil { @@ -244,9 +233,9 @@ func TestStreamDedupBorrowValidatesPeerAddress(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfg, err := NewConfig(WithNodes(tt.nodes), insecureDialOpts, withServer(srv), conn.WithStreamDedup()) + _, closeFn, err := NewConfig(WithNodes(tt.nodes), insecureDialOpts, withServer(srv), conn.WithStreamDedup()) if err == nil { - t.Cleanup(testCloser(t, cfg)) + t.Cleanup(closeFn) t.Fatalf("newConfig succeeded; want error containing %q", tt.wantErr) } if !strings.Contains(err.Error(), tt.wantErr) { @@ -276,7 +265,7 @@ func TestSelfNodeIDStreamRejectedEndToEnd(t *testing.T) { // The client claims the server's own node ID (1). Its stream is rejected, // so the call cannot complete. - cfg := connectAsPeer(t, 1, addrs) + cfg, _ := connectAsPeer(t, 1, addrs) node := cfg.Nodes()[0] // Bound the request itself so it fails fast; wait for that failure on a @@ -343,14 +332,12 @@ func TestKnownPeerConnects(t *testing.T) { func TestKnownPeerDisconnects(t *testing.T) { srv, addrs := testPeerServer(t) - cfg := connectAsPeer(t, 2, addrs) + _, closeFn := connectAsPeer(t, 2, addrs) mustWaitForInbound(t, srv, equalNodeIDs([]uint32{1, 2})) - // Close the configuration to trigger disconnect; Close is idempotent so - // t.Cleanup (registered by connectAsPeer) is harmless. - if err := cfg.Close(); err != nil { - t.Fatalf("cfg.Close() error: %v", err) - } + // Close the configuration to trigger disconnect; the close function is + // idempotent, so t.Cleanup (registered by connectAsPeer) is harmless. + closeFn() mustWaitForInbound(t, srv, equalNodeIDs([]uint32{1})) mock.CheckNodeIDs(t, inboundPeers(srv), []uint32{1}, "after disconnect") } @@ -361,11 +348,11 @@ func TestUnknownPeerIgnored(t *testing.T) { srv, addrs := testPeerServer(t) // Connect without metadata (external client) and with an unknown ID. - cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t)) + _, closeFn, err := NewConfig(WithNodeList(addrs), testDialOptions(t)) if err != nil { t.Fatalf("NewConfig() error: %v", err) } - t.Cleanup(testCloser(t, cfg)) + t.Cleanup(closeFn) connectAsPeer(t, 99, addrs) // ID 99 not in known set @@ -388,11 +375,11 @@ func TestKnownPeerServerCallsClient(t *testing.T) { return NewResponseMessage(in, pb.String("echo: "+req.GetValue())), nil }) peerMD := conn.MetadataWithNodeID(2) - cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithMetadata(peerMD), withServer(clientSrv)) + _, closeFn, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithMetadata(peerMD), withServer(clientSrv)) if err != nil { t.Fatalf("NewConfig() error: %v", err) } - t.Cleanup(testCloser(t, cfg)) + t.Cleanup(closeFn) // Wait for the peer to appear in the inbound config. mustWaitForInbound(t, srv, equalNodeIDs([]uint32{1, 2})) @@ -454,16 +441,17 @@ func testClientServer(t *testing.T) (*Server, []string) { // connectAsPeerClient creates a Config that advertises back-channel // capability by sending the gorums-node-id key (via [withServer]), -// connects to addrs, and returns the configuration. The server will include it in -// ConnectedClients and may dispatch server-initiated calls to it. -func connectAsPeerClient(t *testing.T, addrs []string) Config { +// connects to addrs, and returns the configuration and its close function. The +// server will include it in ConnectedClients and may dispatch server-initiated +// calls to it. +func connectAsPeerClient(t *testing.T, addrs []string) (Config, func()) { t.Helper() - cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t), withServer(NewServer())) + cfg, closeFn, err := NewConfig(WithNodeList(addrs), testDialOptions(t), withServer(NewServer())) if err != nil { t.Fatalf("NewConfig() error: %v", err) } - t.Cleanup(testCloser(t, cfg)) - return cfg + t.Cleanup(closeFn) + return cfg, closeFn } // TestConnectedClientsConnects verifies that a server accepts a peer-capable @@ -492,7 +480,7 @@ func TestConnectedClientsConnects(t *testing.T) { func TestConnectedClientsDisconnects(t *testing.T) { srv, addrs := testClientServer(t) - cfg := connectAsPeerClient(t, addrs) + _, closeFn := connectAsPeerClient(t, addrs) // Wait for the client peer to appear. mustWaitForClients(t, srv, func(cfg Config) bool { return len(cfg) > 0 }) @@ -501,9 +489,7 @@ func TestConnectedClientsDisconnects(t *testing.T) { } // Disconnect the client peer. - if err := cfg.Close(); err != nil { - t.Fatalf("cfg.Close() error: %v", err) - } + closeFn() // Wait for config to become empty. mustWaitForClients(t, srv, func(cfg Config) bool { return len(cfg) == 0 }) @@ -573,11 +559,11 @@ func TestConnectedClientsServerCallsClient(t *testing.T) { wg.Done() return nil, nil }) - clientConfig, err := NewConfig(WithNodeList(addrs), testDialOptions(t), withServer(clientSrv)) + clientConfig, closeFn, err := NewConfig(WithNodeList(addrs), testDialOptions(t), withServer(clientSrv)) if err != nil { t.Fatalf("NewConfig() error: %v", err) } - t.Cleanup(testCloser(t, clientConfig)) + t.Cleanup(closeFn) // Wait for the client to appear in the server's ConnectedClients. mustWaitForClients(t, srv, func(cfg Config) bool { return len(cfg) > 0 }) diff --git a/server_test.go b/server_test.go index e2b60d95..d82d34ca 100644 --- a/server_test.go +++ b/server_test.go @@ -394,7 +394,7 @@ func createServerAndClient(t *testing.T) (*gorums.Server, *gorums.Server, gorums // stream. The client has no node ID, so the server tracks it as an // anonymous client reachable via ConnectedClients. clientSrv := gorums.NewServer() - cfg, err := gorums.NewConfig( + cfg, closeFn, err := gorums.NewConfig( gorums.WithNodeList([]string{lis.Addr().String()}), gorums.WithBackChannel(clientSrv), gorumstest.InsecureDialOptions(t), @@ -410,7 +410,7 @@ func createServerAndClient(t *testing.T) (*gorums.Server, *gorums.Server, gorums // stops the server and closes lis). t.Cleanup(srv.Stop) t.Cleanup(clientSrv.Stop) - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) return srv, clientSrv, cfg } @@ -1115,11 +1115,11 @@ func TestServerBackChannelNestedCallBeforeRelease(t *testing.T) { } return gorums.NewResponseMessage(in, resp), nil }) - cfg, err := gorums.NewConfig(gorums.WithNodeList(addrs), gorumstest.DialOptions(t), gorums.WithBackChannel(clientSrv)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs), gorumstest.DialOptions(t), gorums.WithBackChannel(clientSrv)) if err != nil { t.Fatal(err) } - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) clientCfg = cfg if err := srv.WaitForClients(gorumstest.Context(t, 5*time.Second), func(c gorums.Config) bool { return c.Size() == 1 }); err != nil { @@ -1183,11 +1183,11 @@ func TestServerHandlerNestedCallBeforeRelease(t *testing.T) { clientSrv.RegisterHandler(mock.EchoMethod, func(_ gorums.ServerContext, in *gorums.Message) (*gorums.Message, error) { return gorums.NewResponseMessage(in, pb.String("echo")), nil }) - cfg, err := gorums.NewConfig(gorums.WithNodeList(addrs), gorumstest.DialOptions(t), gorums.WithBackChannel(clientSrv)) + cfg, closeFn, err := gorums.NewConfig(gorums.WithNodeList(addrs), gorumstest.DialOptions(t), gorums.WithBackChannel(clientSrv)) if err != nil { t.Fatal(err) } - t.Cleanup(gorumstest.Closer(t, cfg)) + t.Cleanup(closeFn) if err := srv.WaitForClients(gorumstest.Context(t, 5*time.Second), func(c gorums.Config) bool { return c.Size() == 1 }); err != nil { t.Fatal(err) }