diff --git a/.gitignore b/.gitignore index 368696f20..cd328548b 100644 --- a/.gitignore +++ b/.gitignore @@ -28,7 +28,6 @@ z-scratch/ .scratch/ # Binary files -cmd/benchmark/benchmark examples/storage/storage # Gorums generated backup files diff --git a/AGENTS.md b/AGENTS.md index cd1410bf2..c287c793d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,7 +27,6 @@ gorums/ ├── cmd/protoc-gen-gorums/ # Compiler plugin for code generation │ ├── dev/ # Static code + generated code examples │ └── gengorums/ # Compiler logic + templates -├── benchmark/ # Benchmarking code ├── examples/ # Example implementations ├── internal/ # Internal packages ├── doc/ # Documentation @@ -152,9 +151,6 @@ make -B # Install protoc-gen-gorums plugin make installgorums -# Build benchmark tool -make benchmark - # Install required tools make tools ``` @@ -227,8 +223,6 @@ Before making significant changes, consult: ## Performance Considerations - Gorums is used in performance-critical distributed systems -- Benchmarking tools are available in `benchmark/` and `cmd/benchmark/` -- See `doc/benchmarking.md` for benchmarking procedures - Profile before optimizing - use Go's pprof tools ## Communication with Project Maintainer diff --git a/Makefile b/Makefile index 8481cb28c..eedb33c10 100644 --- a/Makefile +++ b/Makefile @@ -9,11 +9,10 @@ proto_path := $(dev_path):third_party:. plugin_deps := gorums.pb.go $(static_file) runtime_deps := internal/stream/stream.pb.go internal/stream/stream_grpc.pb.go -benchmark_deps := benchmark/benchmark.pb.go benchmark/benchmark_gorums.pb.go -.PHONY: all dev tools bootstrapgorums installgorums benchmark test compiletests genproto benchtest bench +.PHONY: all dev tools bootstrapgorums installgorums test compiletests genproto benchtest bench -all: dev benchmark compiletests +all: dev compiletests dev: installgorums $(runtime_deps) @rm -f $(dev_path)/zorums*.pb.go @@ -23,9 +22,6 @@ dev: installgorums $(runtime_deps) --go_opt=default_api_level=API_OPAQUE \ $(zorums_proto) -benchmark: installgorums $(benchmark_deps) - @go build -o cmd/benchmark/benchmark ./cmd/benchmark - $(static_file): $(static_files) @cp $(static_file) $(static_file).bak @protoc-gen-gorums --bundle=$(static_file) @@ -104,12 +100,11 @@ stressgen: tools modernize: @go run golang.org/x/tools/go/analysis/passes/modernize/cmd/modernize@latest -fix ./... -# Regenerate all Gorums and protobuf generated files across the repo (dev, benchmark, internal/tests, examples). +# Regenerate all Gorums and protobuf generated files across the repo (dev, internal/tests, examples). # This will force regeneration even though the proto files have not changed. genproto: installgorums dev - @echo "Regenerating all proto files (dev, benchmark, internal/tests, examples)" + @echo "Regenerating all proto files (dev, internal/tests, examples)" @$(MAKE) -B -s dev - @$(MAKE) -B -s benchmark @$(MAKE) -B -s --no-print-directory -C ./internal/tests all @$(MAKE) -B -s --no-print-directory -C ./examples all diff --git a/benchmark/benchmark.go b/benchmark/benchmark.go deleted file mode 100644 index adf0b4efa..000000000 --- a/benchmark/benchmark.go +++ /dev/null @@ -1,293 +0,0 @@ -package benchmark - -import ( - context "context" - "maps" - "regexp" - "runtime" - "slices" - "sort" - "sync/atomic" - "time" - - "github.com/relab/gorums" - "golang.org/x/sync/errgroup" -) - -// Options controls different options for the benchmarks -type Options struct { - Concurrent int // Number of concurrent calls - Duration time.Duration // Duration of benchmark - MaxAsync int // Max async calls at once - NumNodes int // Number of nodes to include in configuration - Payload int // Size of message payload - QuorumSize int // Number of messages to wait for - Warmup time.Duration // Warmup time - Remote bool // Whether the servers are remote (true) or local (false) -} - -// Bench is a Benchmark with a name and description -type Bench struct { - Name string - Description string - runBench benchFunc -} - -type ( - benchFunc func(Options) (*Result, error) - qcFunc func(*ConfigContext, *Echo, int, ...gorums.CallOption) (*Echo, error) - asyncQCFunc func(*ConfigContext, *Echo, int, ...gorums.CallOption) AsyncEcho - serverFunc func(context.Context, *TimedMsg) -) - -func runQCBenchmark(opts Options, config Configuration, f qcFunc) (*Result, error) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - cfgCtx := config.Context(ctx) - msg := Echo_builder{Payload: make([]byte, opts.Payload)}.Build() - s := &Stats{} - var g errgroup.Group - - for range opts.Concurrent { - g.Go(func() error { - warmupEnd := time.Now().Add(opts.Warmup) - for !time.Now().After(warmupEnd) { - _, err := f(cfgCtx, msg, opts.QuorumSize) - if err != nil { - return err - } - } - return nil - }) - } - if err := g.Wait(); err != nil { - return nil, err - } - - if opts.Remote { - _, err := StartBenchmark(cfgCtx, &StartRequest{}).All() - if err != nil { - return nil, err - } - } - - s.Start() - for range opts.Concurrent { - g.Go(func() error { - endTime := time.Now().Add(opts.Duration) - for !time.Now().After(endTime) { - start := time.Now() - _, err := f(cfgCtx, msg, opts.QuorumSize) - if err != nil { - return err - } - s.AddLatency(time.Since(start)) - } - return nil - }) - } - if err := g.Wait(); err != nil { - return nil, err - } - s.End() - - result := s.GetResult() - if opts.Remote { - replies := StopBenchmark(cfgCtx, &StopRequest{}).Results().CollectAll() - result.SetServerStats(slices.Collect(maps.Values(replies))) - } - - return result, nil -} - -func runAsyncQCBenchmark(opts Options, config Configuration, f asyncQCFunc) (*Result, error) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - cfgCtx := config.Context(ctx) - msg := Echo_builder{Payload: make([]byte, opts.Payload)}.Build() - s := &Stats{} - var g errgroup.Group - - warmupEnd := time.Now().Add(opts.Warmup) - var async uint64 - - var warmupFunc func() error - warmupFunc = func() error { - for ; !time.Now().After(warmupEnd) && atomic.LoadUint64(&async) < uint64(opts.MaxAsync); atomic.AddUint64(&async, 1) { - fut := f(cfgCtx, msg, opts.QuorumSize) - g.Go(func() error { - _, err := fut.Get() - if err != nil { - return err - } - atomic.AddUint64(&async, ^uint64(0)) - _ = warmupFunc() - return nil - }) - } - return nil - } - - for range opts.Concurrent { - g.Go(warmupFunc) - } - if err := g.Wait(); err != nil { - return nil, err - } - - if opts.Remote { - _, err := StartBenchmark(cfgCtx, &StartRequest{}).All() - if err != nil { - return nil, err - } - } - - endTime := time.Now().Add(opts.Duration) - var benchmarkFunc func() error - benchmarkFunc = func() error { - for ; !time.Now().After(endTime) && atomic.LoadUint64(&async) < uint64(opts.MaxAsync); atomic.AddUint64(&async, 1) { - start := time.Now() - fut := f(cfgCtx, msg, opts.QuorumSize) - g.Go(func() error { - _, err := fut.Get() - if err != nil { - return err - } - s.AddLatency(time.Since(start)) - atomic.AddUint64(&async, ^uint64(0)) - _ = benchmarkFunc() - return nil - }) - } - return nil - } - - s.Start() - for range opts.Concurrent { - g.Go(benchmarkFunc) - } - if err := g.Wait(); err != nil { - return nil, err - } - s.End() - - result := s.GetResult() - if opts.Remote { - replies := StopBenchmark(cfgCtx, &StopRequest{}).Results().CollectAll() - result.SetServerStats(slices.Collect(maps.Values(replies))) - } - - return result, nil -} - -func runServerBenchmark(opts Options, config Configuration, f serverFunc) (*Result, error) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - cfgCtx := config.Context(ctx) - payload := make([]byte, opts.Payload) - var start runtime.MemStats - var end runtime.MemStats - - benchmarkFunc := func(stopTime time.Time) { - for !time.Now().After(stopTime) { - msg := TimedMsg_builder{SendTime: time.Now().UnixNano(), Payload: payload}.Build() - f(ctx, msg) - } - } - - warmupEnd := time.Now().Add(opts.Warmup) - for range opts.Concurrent { - benchmarkFunc(warmupEnd) - } - - _, err := StartServerBenchmark(cfgCtx, &StartRequest{}).All() - if err != nil { - return nil, err - } - runtime.ReadMemStats(&start) - - endTime := time.Now().Add(opts.Duration) - for range opts.Concurrent { - benchmarkFunc(endTime) - } - runtime.ReadMemStats(&end) - - replies := StopServerBenchmark(cfgCtx, &StopRequest{}).Results().CollectAll() - resp, err := StopServerBenchmarkQF(replies) - if err != nil { - return nil, err - } - - clientAllocs := (end.Mallocs - start.Mallocs) / resp.GetTotalOps() - clientMem := (end.TotalAlloc - start.TotalAlloc) / resp.GetTotalOps() - - resp.SetAllocsPerOp(clientAllocs) - resp.SetMemPerOp(clientMem) - return resp, nil -} - -// GetBenchmarks returns a list of Benchmarks that can be performed on the configuration -func GetBenchmarks(config Configuration) []Bench { - m := []Bench{ - { - Name: "QuorumCall", - Description: "NodeStream based quorum call implementation with FIFO ordering", - runBench: func(opts Options) (*Result, error) { - return runQCBenchmark(opts, config, func(ctx *ConfigContext, in *Echo, quorumSize int, callOpts ...gorums.CallOption) (*Echo, error) { - return QuorumCall(ctx, in, callOpts...).Threshold(quorumSize) - }) - }, - }, - { - Name: "AsyncQuorumCall", - Description: "NodeStream based async quorum call implementation with FIFO ordering", - runBench: func(opts Options) (*Result, error) { - return runAsyncQCBenchmark(opts, config, func(ctx *ConfigContext, in *Echo, quorumSize int, callOpts ...gorums.CallOption) AsyncEcho { - return QuorumCall(ctx, in, callOpts...).AsyncThreshold(quorumSize) - }) - }, - }, - { - Name: "SlowServer", - Description: "Quorum Call with a 10s processing time on the server", - runBench: func(opts Options) (*Result, error) { - return runQCBenchmark(opts, config, func(ctx *ConfigContext, in *Echo, quorumSize int, callOpts ...gorums.CallOption) (*Echo, error) { - return SlowServer(ctx, in, callOpts...).Threshold(quorumSize) - }) - }, - }, - { - Name: "Multicast", - Description: "NodeStream based multicast implementation (servers measure latency and throughput)", - runBench: func(opts Options) (*Result, error) { - return runServerBenchmark(opts, config, func(ctx context.Context, msg *TimedMsg) { - cfgCtx := config.Context(ctx) - Multicast(cfgCtx, msg, gorums.IgnoreErrors()) - }) - }, - }, - } - return m -} - -// RunBenchmarks runs all the benchmarks that match the given regex with the given options -func RunBenchmarks(benchRegex *regexp.Regexp, options Options, config Configuration) ([]*Result, error) { - benchmarks := GetBenchmarks(config) - var results []*Result - for _, b := range benchmarks { - if benchRegex.MatchString(b.Name) { - result, err := b.runBench(options) - if err != nil { - return nil, err - } - result.SetName(b.Name) - i := sort.Search(len(results), func(i int) bool { - return results[i].GetName() >= result.GetName() - }) - results = append(results, nil) - copy(results[i+1:], results[i:]) - results[i] = result - } - } - return results, nil -} diff --git a/benchmark/benchmark.pb.go b/benchmark/benchmark.pb.go deleted file mode 100644 index 01f06b9cf..000000000 --- a/benchmark/benchmark.pb.go +++ /dev/null @@ -1,637 +0,0 @@ -// Code generated by protoc-gen-go. DO NOT EDIT. -// versions: -// protoc-gen-go v1.36.11 -// protoc v7.34.1 -// source: benchmark/benchmark.proto - -package benchmark - -import ( - _ "github.com/relab/gorums" - protoreflect "google.golang.org/protobuf/reflect/protoreflect" - protoimpl "google.golang.org/protobuf/runtime/protoimpl" - emptypb "google.golang.org/protobuf/types/known/emptypb" - reflect "reflect" - unsafe "unsafe" -) - -const ( - // Verify that this generated code is sufficiently up-to-date. - _ = protoimpl.EnforceVersion(20 - protoimpl.MinVersion) - // Verify that runtime/protoimpl is sufficiently up-to-date. - _ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20) -) - -// Echo is a simple message used for echo benchmarks. -type Echo struct { - state protoimpl.MessageState `protogen:"opaque.v1"` - xxx_hidden_Payload []byte `protobuf:"bytes,1,opt,name=payload"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache -} - -func (x *Echo) Reset() { - *x = Echo{} - mi := &file_benchmark_benchmark_proto_msgTypes[0] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) -} - -func (x *Echo) String() string { - return protoimpl.X.MessageStringOf(x) -} - -func (*Echo) ProtoMessage() {} - -func (x *Echo) ProtoReflect() protoreflect.Message { - mi := &file_benchmark_benchmark_proto_msgTypes[0] - if x != nil { - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) - } - return ms - } - return mi.MessageOf(x) -} - -func (x *Echo) GetPayload() []byte { - if x != nil { - return x.xxx_hidden_Payload - } - return nil -} - -func (x *Echo) SetPayload(v []byte) { - if v == nil { - v = []byte{} - } - x.xxx_hidden_Payload = v -} - -type Echo_builder struct { - _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. - - Payload []byte -} - -func (b0 Echo_builder) Build() *Echo { - m0 := &Echo{} - b, x := &b0, m0 - _, _ = b, x - x.xxx_hidden_Payload = b.Payload - return m0 -} - -// TimedMsg is a message with a send time and a payload used for multicast benchmarks. -type TimedMsg struct { - state protoimpl.MessageState `protogen:"opaque.v1"` - xxx_hidden_SendTime int64 `protobuf:"varint,1,opt,name=send_time,json=sendTime"` - xxx_hidden_Payload []byte `protobuf:"bytes,2,opt,name=payload"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache -} - -func (x *TimedMsg) Reset() { - *x = TimedMsg{} - mi := &file_benchmark_benchmark_proto_msgTypes[1] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) -} - -func (x *TimedMsg) String() string { - return protoimpl.X.MessageStringOf(x) -} - -func (*TimedMsg) ProtoMessage() {} - -func (x *TimedMsg) ProtoReflect() protoreflect.Message { - mi := &file_benchmark_benchmark_proto_msgTypes[1] - if x != nil { - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) - } - return ms - } - return mi.MessageOf(x) -} - -func (x *TimedMsg) GetSendTime() int64 { - if x != nil { - return x.xxx_hidden_SendTime - } - return 0 -} - -func (x *TimedMsg) GetPayload() []byte { - if x != nil { - return x.xxx_hidden_Payload - } - return nil -} - -func (x *TimedMsg) SetSendTime(v int64) { - x.xxx_hidden_SendTime = v -} - -func (x *TimedMsg) SetPayload(v []byte) { - if v == nil { - v = []byte{} - } - x.xxx_hidden_Payload = v -} - -type TimedMsg_builder struct { - _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. - - SendTime int64 - Payload []byte -} - -func (b0 TimedMsg_builder) Build() *TimedMsg { - m0 := &TimedMsg{} - b, x := &b0, m0 - _, _ = b, x - x.xxx_hidden_SendTime = b.SendTime - x.xxx_hidden_Payload = b.Payload - return m0 -} - -// StartRequest is an empty message for starting a benchmarking campaign. -type StartRequest struct { - state protoimpl.MessageState `protogen:"opaque.v1"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache -} - -func (x *StartRequest) Reset() { - *x = StartRequest{} - mi := &file_benchmark_benchmark_proto_msgTypes[2] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) -} - -func (x *StartRequest) String() string { - return protoimpl.X.MessageStringOf(x) -} - -func (*StartRequest) ProtoMessage() {} - -func (x *StartRequest) ProtoReflect() protoreflect.Message { - mi := &file_benchmark_benchmark_proto_msgTypes[2] - if x != nil { - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) - } - return ms - } - return mi.MessageOf(x) -} - -type StartRequest_builder struct { - _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. - -} - -func (b0 StartRequest_builder) Build() *StartRequest { - m0 := &StartRequest{} - b, x := &b0, m0 - _, _ = b, x - return m0 -} - -// StartResponse is an empty message to acknowledge the start of a benchmarking campaign. -type StartResponse struct { - state protoimpl.MessageState `protogen:"opaque.v1"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache -} - -func (x *StartResponse) Reset() { - *x = StartResponse{} - mi := &file_benchmark_benchmark_proto_msgTypes[3] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) -} - -func (x *StartResponse) String() string { - return protoimpl.X.MessageStringOf(x) -} - -func (*StartResponse) ProtoMessage() {} - -func (x *StartResponse) ProtoReflect() protoreflect.Message { - mi := &file_benchmark_benchmark_proto_msgTypes[3] - if x != nil { - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) - } - return ms - } - return mi.MessageOf(x) -} - -type StartResponse_builder struct { - _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. - -} - -func (b0 StartResponse_builder) Build() *StartResponse { - m0 := &StartResponse{} - b, x := &b0, m0 - _, _ = b, x - return m0 -} - -// StopRequest is an empty message for stopping a benchmarking campaign. -type StopRequest struct { - state protoimpl.MessageState `protogen:"opaque.v1"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache -} - -func (x *StopRequest) Reset() { - *x = StopRequest{} - mi := &file_benchmark_benchmark_proto_msgTypes[4] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) -} - -func (x *StopRequest) String() string { - return protoimpl.X.MessageStringOf(x) -} - -func (*StopRequest) ProtoMessage() {} - -func (x *StopRequest) ProtoReflect() protoreflect.Message { - mi := &file_benchmark_benchmark_proto_msgTypes[4] - if x != nil { - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) - } - return ms - } - return mi.MessageOf(x) -} - -type StopRequest_builder struct { - _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. - -} - -func (b0 StopRequest_builder) Build() *StopRequest { - m0 := &StopRequest{} - b, x := &b0, m0 - _, _ = b, x - return m0 -} - -// Result contains the results of a server-side benchmarking campaign. -type Result struct { - state protoimpl.MessageState `protogen:"opaque.v1"` - xxx_hidden_Name string `protobuf:"bytes,1,opt,name=name"` - xxx_hidden_TotalOps uint64 `protobuf:"varint,2,opt,name=total_ops,json=totalOps"` - xxx_hidden_TotalTime int64 `protobuf:"varint,3,opt,name=total_time,json=totalTime"` - xxx_hidden_Throughput float64 `protobuf:"fixed64,4,opt,name=throughput"` - xxx_hidden_LatencyAvg float64 `protobuf:"fixed64,5,opt,name=latency_avg,json=latencyAvg"` - xxx_hidden_LatencyVar float64 `protobuf:"fixed64,6,opt,name=latency_var,json=latencyVar"` - xxx_hidden_AllocsPerOp uint64 `protobuf:"varint,7,opt,name=allocs_per_op,json=allocsPerOp"` - xxx_hidden_MemPerOp uint64 `protobuf:"varint,8,opt,name=mem_per_op,json=memPerOp"` - xxx_hidden_ServerStats *[]*MemoryStat `protobuf:"bytes,9,rep,name=server_stats,json=serverStats"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache -} - -func (x *Result) Reset() { - *x = Result{} - mi := &file_benchmark_benchmark_proto_msgTypes[5] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) -} - -func (x *Result) String() string { - return protoimpl.X.MessageStringOf(x) -} - -func (*Result) ProtoMessage() {} - -func (x *Result) ProtoReflect() protoreflect.Message { - mi := &file_benchmark_benchmark_proto_msgTypes[5] - if x != nil { - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) - } - return ms - } - return mi.MessageOf(x) -} - -func (x *Result) GetName() string { - if x != nil { - return x.xxx_hidden_Name - } - return "" -} - -func (x *Result) GetTotalOps() uint64 { - if x != nil { - return x.xxx_hidden_TotalOps - } - return 0 -} - -func (x *Result) GetTotalTime() int64 { - if x != nil { - return x.xxx_hidden_TotalTime - } - return 0 -} - -func (x *Result) GetThroughput() float64 { - if x != nil { - return x.xxx_hidden_Throughput - } - return 0 -} - -func (x *Result) GetLatencyAvg() float64 { - if x != nil { - return x.xxx_hidden_LatencyAvg - } - return 0 -} - -func (x *Result) GetLatencyVar() float64 { - if x != nil { - return x.xxx_hidden_LatencyVar - } - return 0 -} - -func (x *Result) GetAllocsPerOp() uint64 { - if x != nil { - return x.xxx_hidden_AllocsPerOp - } - return 0 -} - -func (x *Result) GetMemPerOp() uint64 { - if x != nil { - return x.xxx_hidden_MemPerOp - } - return 0 -} - -func (x *Result) GetServerStats() []*MemoryStat { - if x != nil { - if x.xxx_hidden_ServerStats != nil { - return *x.xxx_hidden_ServerStats - } - } - return nil -} - -func (x *Result) SetName(v string) { - x.xxx_hidden_Name = v -} - -func (x *Result) SetTotalOps(v uint64) { - x.xxx_hidden_TotalOps = v -} - -func (x *Result) SetTotalTime(v int64) { - x.xxx_hidden_TotalTime = v -} - -func (x *Result) SetThroughput(v float64) { - x.xxx_hidden_Throughput = v -} - -func (x *Result) SetLatencyAvg(v float64) { - x.xxx_hidden_LatencyAvg = v -} - -func (x *Result) SetLatencyVar(v float64) { - x.xxx_hidden_LatencyVar = v -} - -func (x *Result) SetAllocsPerOp(v uint64) { - x.xxx_hidden_AllocsPerOp = v -} - -func (x *Result) SetMemPerOp(v uint64) { - x.xxx_hidden_MemPerOp = v -} - -func (x *Result) SetServerStats(v []*MemoryStat) { - x.xxx_hidden_ServerStats = &v -} - -type Result_builder struct { - _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. - - Name string - TotalOps uint64 - TotalTime int64 - Throughput float64 - LatencyAvg float64 - LatencyVar float64 - AllocsPerOp uint64 - MemPerOp uint64 - ServerStats []*MemoryStat -} - -func (b0 Result_builder) Build() *Result { - m0 := &Result{} - b, x := &b0, m0 - _, _ = b, x - x.xxx_hidden_Name = b.Name - x.xxx_hidden_TotalOps = b.TotalOps - x.xxx_hidden_TotalTime = b.TotalTime - x.xxx_hidden_Throughput = b.Throughput - x.xxx_hidden_LatencyAvg = b.LatencyAvg - x.xxx_hidden_LatencyVar = b.LatencyVar - x.xxx_hidden_AllocsPerOp = b.AllocsPerOp - x.xxx_hidden_MemPerOp = b.MemPerOp - x.xxx_hidden_ServerStats = &b.ServerStats - return m0 -} - -// MemoryStat contains memory statistics for a single server. -type MemoryStat struct { - state protoimpl.MessageState `protogen:"opaque.v1"` - xxx_hidden_Allocs uint64 `protobuf:"varint,1,opt,name=allocs"` - xxx_hidden_Memory uint64 `protobuf:"varint,2,opt,name=memory"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache -} - -func (x *MemoryStat) Reset() { - *x = MemoryStat{} - mi := &file_benchmark_benchmark_proto_msgTypes[6] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) -} - -func (x *MemoryStat) String() string { - return protoimpl.X.MessageStringOf(x) -} - -func (*MemoryStat) ProtoMessage() {} - -func (x *MemoryStat) ProtoReflect() protoreflect.Message { - mi := &file_benchmark_benchmark_proto_msgTypes[6] - if x != nil { - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) - } - return ms - } - return mi.MessageOf(x) -} - -func (x *MemoryStat) GetAllocs() uint64 { - if x != nil { - return x.xxx_hidden_Allocs - } - return 0 -} - -func (x *MemoryStat) GetMemory() uint64 { - if x != nil { - return x.xxx_hidden_Memory - } - return 0 -} - -func (x *MemoryStat) SetAllocs(v uint64) { - x.xxx_hidden_Allocs = v -} - -func (x *MemoryStat) SetMemory(v uint64) { - x.xxx_hidden_Memory = v -} - -type MemoryStat_builder struct { - _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. - - Allocs uint64 - Memory uint64 -} - -func (b0 MemoryStat_builder) Build() *MemoryStat { - m0 := &MemoryStat{} - b, x := &b0, m0 - _, _ = b, x - x.xxx_hidden_Allocs = b.Allocs - x.xxx_hidden_Memory = b.Memory - return m0 -} - -var File_benchmark_benchmark_proto protoreflect.FileDescriptor - -const file_benchmark_benchmark_proto_rawDesc = "" + - "\n" + - "\x19benchmark/benchmark.proto\x12\tbenchmark\x1a\x1bgoogle/protobuf/empty.proto\x1a\fgorums.proto\" \n" + - "\x04Echo\x12\x18\n" + - "\apayload\x18\x01 \x01(\fR\apayload\"A\n" + - "\bTimedMsg\x12\x1b\n" + - "\tsend_time\x18\x01 \x01(\x03R\bsendTime\x12\x18\n" + - "\apayload\x18\x02 \x01(\fR\apayload\"\x0e\n" + - "\fStartRequest\"\x0f\n" + - "\rStartResponse\"\r\n" + - "\vStopRequest\"\xb6\x02\n" + - "\x06Result\x12\x12\n" + - "\x04name\x18\x01 \x01(\tR\x04name\x12\x1b\n" + - "\ttotal_ops\x18\x02 \x01(\x04R\btotalOps\x12\x1d\n" + - "\n" + - "total_time\x18\x03 \x01(\x03R\ttotalTime\x12\x1e\n" + - "\n" + - "throughput\x18\x04 \x01(\x01R\n" + - "throughput\x12\x1f\n" + - "\vlatency_avg\x18\x05 \x01(\x01R\n" + - "latencyAvg\x12\x1f\n" + - "\vlatency_var\x18\x06 \x01(\x01R\n" + - "latencyVar\x12\"\n" + - "\rallocs_per_op\x18\a \x01(\x04R\vallocsPerOp\x12\x1c\n" + - "\n" + - "mem_per_op\x18\b \x01(\x04R\bmemPerOp\x128\n" + - "\fserver_stats\x18\t \x03(\v2\x15.benchmark.MemoryStatR\vserverStats\"<\n" + - "\n" + - "MemoryStat\x12\x16\n" + - "\x06allocs\x18\x01 \x01(\x04R\x06allocs\x12\x16\n" + - "\x06memory\x18\x02 \x01(\x04R\x06memory2\xe1\x03\n" + - "\tBenchmark\x12O\n" + - "\x14StartServerBenchmark\x12\x17.benchmark.StartRequest\x1a\x18.benchmark.StartResponse\"\x04\xa0\xb5\x18\x01\x12F\n" + - "\x13StopServerBenchmark\x12\x16.benchmark.StopRequest\x1a\x11.benchmark.Result\"\x04\xa0\xb5\x18\x01\x12I\n" + - "\x0eStartBenchmark\x12\x17.benchmark.StartRequest\x1a\x18.benchmark.StartResponse\"\x04\xa0\xb5\x18\x01\x12D\n" + - "\rStopBenchmark\x12\x16.benchmark.StopRequest\x1a\x15.benchmark.MemoryStat\"\x04\xa0\xb5\x18\x01\x124\n" + - "\n" + - "QuorumCall\x12\x0f.benchmark.Echo\x1a\x0f.benchmark.Echo\"\x04\xa0\xb5\x18\x01\x124\n" + - "\n" + - "SlowServer\x12\x0f.benchmark.Echo\x1a\x0f.benchmark.Echo\"\x04\xa0\xb5\x18\x01\x12>\n" + - "\tMulticast\x12\x13.benchmark.TimedMsg\x1a\x16.google.protobuf.Empty\"\x04\x98\xb5\x18\x01B(Z!github.com/relab/gorums/benchmark\x92\x03\x02\b\x02b\beditionsp\xe9\a" - -var file_benchmark_benchmark_proto_msgTypes = make([]protoimpl.MessageInfo, 7) -var file_benchmark_benchmark_proto_goTypes = []any{ - (*Echo)(nil), // 0: benchmark.Echo - (*TimedMsg)(nil), // 1: benchmark.TimedMsg - (*StartRequest)(nil), // 2: benchmark.StartRequest - (*StartResponse)(nil), // 3: benchmark.StartResponse - (*StopRequest)(nil), // 4: benchmark.StopRequest - (*Result)(nil), // 5: benchmark.Result - (*MemoryStat)(nil), // 6: benchmark.MemoryStat - (*emptypb.Empty)(nil), // 7: google.protobuf.Empty -} -var file_benchmark_benchmark_proto_depIdxs = []int32{ - 6, // 0: benchmark.Result.server_stats:type_name -> benchmark.MemoryStat - 2, // 1: benchmark.Benchmark.StartServerBenchmark:input_type -> benchmark.StartRequest - 4, // 2: benchmark.Benchmark.StopServerBenchmark:input_type -> benchmark.StopRequest - 2, // 3: benchmark.Benchmark.StartBenchmark:input_type -> benchmark.StartRequest - 4, // 4: benchmark.Benchmark.StopBenchmark:input_type -> benchmark.StopRequest - 0, // 5: benchmark.Benchmark.QuorumCall:input_type -> benchmark.Echo - 0, // 6: benchmark.Benchmark.SlowServer:input_type -> benchmark.Echo - 1, // 7: benchmark.Benchmark.Multicast:input_type -> benchmark.TimedMsg - 3, // 8: benchmark.Benchmark.StartServerBenchmark:output_type -> benchmark.StartResponse - 5, // 9: benchmark.Benchmark.StopServerBenchmark:output_type -> benchmark.Result - 3, // 10: benchmark.Benchmark.StartBenchmark:output_type -> benchmark.StartResponse - 6, // 11: benchmark.Benchmark.StopBenchmark:output_type -> benchmark.MemoryStat - 0, // 12: benchmark.Benchmark.QuorumCall:output_type -> benchmark.Echo - 0, // 13: benchmark.Benchmark.SlowServer:output_type -> benchmark.Echo - 7, // 14: benchmark.Benchmark.Multicast:output_type -> google.protobuf.Empty - 8, // [8:15] is the sub-list for method output_type - 1, // [1:8] is the sub-list for method input_type - 1, // [1:1] is the sub-list for extension type_name - 1, // [1:1] is the sub-list for extension extendee - 0, // [0:1] is the sub-list for field type_name -} - -func init() { file_benchmark_benchmark_proto_init() } -func file_benchmark_benchmark_proto_init() { - if File_benchmark_benchmark_proto != nil { - return - } - type x struct{} - out := protoimpl.TypeBuilder{ - File: protoimpl.DescBuilder{ - GoPackagePath: reflect.TypeOf(x{}).PkgPath(), - RawDescriptor: unsafe.Slice(unsafe.StringData(file_benchmark_benchmark_proto_rawDesc), len(file_benchmark_benchmark_proto_rawDesc)), - NumEnums: 0, - NumMessages: 7, - NumExtensions: 0, - NumServices: 1, - }, - GoTypes: file_benchmark_benchmark_proto_goTypes, - DependencyIndexes: file_benchmark_benchmark_proto_depIdxs, - MessageInfos: file_benchmark_benchmark_proto_msgTypes, - }.Build() - File_benchmark_benchmark_proto = out.File - file_benchmark_benchmark_proto_goTypes = nil - file_benchmark_benchmark_proto_depIdxs = nil -} diff --git a/benchmark/benchmark.proto b/benchmark/benchmark.proto deleted file mode 100644 index fcf76a430..000000000 --- a/benchmark/benchmark.proto +++ /dev/null @@ -1,88 +0,0 @@ -edition = "2024"; - -package benchmark; -option go_package = "github.com/relab/gorums/benchmark"; -option features.field_presence = IMPLICIT; - -import "google/protobuf/empty.proto"; - -import "gorums.proto"; - -// Echo is a simple message used for echo benchmarks. -message Echo { - bytes payload = 1; -} - -// TimedMsg is a message with a send time and a payload used for multicast benchmarks. -message TimedMsg { - int64 send_time = 1; - bytes payload = 2; -} - -// StartRequest is an empty message for starting a benchmarking campaign. -message StartRequest {} - -// StartResponse is an empty message to acknowledge the start of a benchmarking campaign. -message StartResponse {} - -// StopRequest is an empty message for stopping a benchmarking campaign. -message StopRequest {} - -// Result contains the results of a server-side benchmarking campaign. -message Result { - string name = 1; - uint64 total_ops = 2; - int64 total_time = 3; - double throughput = 4; - double latency_avg = 5; - double latency_var = 6; - uint64 allocs_per_op = 7; - uint64 mem_per_op = 8; - repeated MemoryStat server_stats = 9; -} - -// MemoryStat contains memory statistics for a single server. -message MemoryStat { - uint64 allocs = 1; - uint64 memory = 2; -} - -// Benchmark is a service for running various benchmarks. -service Benchmark { - // StartServerBenchmark starts a server-side benchmark campaign. - rpc StartServerBenchmark(StartRequest) returns (StartResponse) { - option (gorums.quorumcall) = true; - } - - // StopServerBenchmark stops a server-side benchmark campaign. - rpc StopServerBenchmark(StopRequest) returns (Result) { - option (gorums.quorumcall) = true; - } - - // StartBenchmark starts a client-side benchmark campaign. - rpc StartBenchmark(StartRequest) returns (StartResponse) { - option (gorums.quorumcall) = true; - } - - // StopBenchmark stops a client-side benchmark campaign. - rpc StopBenchmark(StopRequest) returns (MemoryStat) { - option (gorums.quorumcall) = true; - } - - // That actual benchmark RPCs: - - // QuorumCall performs an echo quorum call on all servers. - rpc QuorumCall(Echo) returns (Echo) { - option (gorums.quorumcall) = true; - } - - // SlowServer performs an echo quorum call on slow servers. - rpc SlowServer(Echo) returns (Echo) { - option (gorums.quorumcall) = true; - } - - // Multicast performs a multicast call to all servers. - rpc Multicast(TimedMsg) returns (google.protobuf.Empty) { - option (gorums.multicast) = true; - } -} diff --git a/benchmark/benchmark_gorums.pb.go b/benchmark/benchmark_gorums.pb.go deleted file mode 100644 index a7b6c9103..000000000 --- a/benchmark/benchmark_gorums.pb.go +++ /dev/null @@ -1,186 +0,0 @@ -// Code generated by protoc-gen-gorums. DO NOT EDIT. -// versions: -// protoc-gen-gorums v0.11.0-devel -// protoc v7.34.1 -// source: benchmark/benchmark.proto - -package benchmark - -import ( - gorums "github.com/relab/gorums" - emptypb "google.golang.org/protobuf/types/known/emptypb" -) - -const ( - // Verify that this generated code is sufficiently up-to-date. - _ = gorums.EnforceVersion(11 - gorums.MinVersion) - // Verify that the gorums runtime is sufficiently up-to-date. - _ = gorums.EnforceVersion(gorums.MaxVersion - 11) -) - -// The type aliases below are useful Gorums types that we make accessible -// from generated code. These names therefore become reserved identifiers, -// meaning that proto message types with these names would collide with the -// generated aliases and cause a compile error. -// -// The bundler (gorums_bundle.go) is responsible for discovering these -// aliases and any other identifiers defined herein, and adding them to -// the reserved identifiers list. -// -// If necessary, additional aliases and other identifiers should be added in -// the generator's cmd/protoc-gen-gorums/dev directory, and the bundler will -// automatically discover them and add them to the reserved identifiers list. - -type ( - Configuration = gorums.Configuration - Node = gorums.Node - NodeContext = gorums.NodeContext - ConfigContext = gorums.ConfigContext -) - -// AsyncEcho is a future for async quorum calls returning *Echo. -type AsyncEcho = *gorums.Async[*Echo] - -// AsyncMemoryStat is a future for async quorum calls returning *MemoryStat. -type AsyncMemoryStat = *gorums.Async[*MemoryStat] - -// AsyncResult is a future for async quorum calls returning *Result. -type AsyncResult = *gorums.Async[*Result] - -// AsyncStartResponse is a future for async quorum calls returning *StartResponse. -type AsyncStartResponse = *gorums.Async[*StartResponse] - -// CorrectableEcho is a correctable object for quorum calls returning *Echo. -type CorrectableEcho = *gorums.Correctable[*Echo] - -// CorrectableMemoryStat is a correctable object for quorum calls returning *MemoryStat. -type CorrectableMemoryStat = *gorums.Correctable[*MemoryStat] - -// CorrectableResult is a correctable object for quorum calls returning *Result. -type CorrectableResult = *gorums.Correctable[*Result] - -// CorrectableStartResponse is a correctable object for quorum calls returning *StartResponse. -type CorrectableStartResponse = *gorums.Correctable[*StartResponse] - -// Reference imports to suppress errors if they are not otherwise used. -var _ emptypb.Empty - -// StartServerBenchmark starts a server-side benchmark campaign. -func StartServerBenchmark(ctx *ConfigContext, in *StartRequest, opts ...gorums.CallOption) *gorums.Responses[*StartResponse] { - return gorums.QuorumCall[*StartRequest, *StartResponse]( - ctx, in, "benchmark.Benchmark.StartServerBenchmark", - opts..., - ) -} - -// StopServerBenchmark stops a server-side benchmark campaign. -func StopServerBenchmark(ctx *ConfigContext, in *StopRequest, opts ...gorums.CallOption) *gorums.Responses[*Result] { - return gorums.QuorumCall[*StopRequest, *Result]( - ctx, in, "benchmark.Benchmark.StopServerBenchmark", - opts..., - ) -} - -// StartBenchmark starts a client-side benchmark campaign. -func StartBenchmark(ctx *ConfigContext, in *StartRequest, opts ...gorums.CallOption) *gorums.Responses[*StartResponse] { - return gorums.QuorumCall[*StartRequest, *StartResponse]( - ctx, in, "benchmark.Benchmark.StartBenchmark", - opts..., - ) -} - -// StopBenchmark stops a client-side benchmark campaign. -func StopBenchmark(ctx *ConfigContext, in *StopRequest, opts ...gorums.CallOption) *gorums.Responses[*MemoryStat] { - return gorums.QuorumCall[*StopRequest, *MemoryStat]( - ctx, in, "benchmark.Benchmark.StopBenchmark", - opts..., - ) -} - -// QuorumCall performs an echo quorum call on all servers. -func QuorumCall(ctx *ConfigContext, in *Echo, opts ...gorums.CallOption) *gorums.Responses[*Echo] { - return gorums.QuorumCall[*Echo, *Echo]( - ctx, in, "benchmark.Benchmark.QuorumCall", - opts..., - ) -} - -// SlowServer performs an echo quorum call on slow servers. -func SlowServer(ctx *ConfigContext, in *Echo, opts ...gorums.CallOption) *gorums.Responses[*Echo] { - return gorums.QuorumCall[*Echo, *Echo]( - ctx, in, "benchmark.Benchmark.SlowServer", - opts..., - ) -} - -// Multicast performs a multicast call to all servers. -func Multicast(ctx *ConfigContext, in *TimedMsg, opts ...gorums.CallOption) error { - return gorums.Multicast(ctx, in, "benchmark.Benchmark.Multicast", opts...) -} - -// Benchmark is the server-side API for the Benchmark Service -type BenchmarkServer interface { - StartServerBenchmark(gorums.ServerCtx, *StartRequest) (*StartResponse, error) - StopServerBenchmark(gorums.ServerCtx, *StopRequest) (*Result, error) - StartBenchmark(gorums.ServerCtx, *StartRequest) (*StartResponse, error) - StopBenchmark(gorums.ServerCtx, *StopRequest) (*MemoryStat, error) - QuorumCall(gorums.ServerCtx, *Echo) (*Echo, error) - SlowServer(gorums.ServerCtx, *Echo) (*Echo, error) - Multicast(gorums.ServerCtx, *TimedMsg) -} - -func RegisterBenchmarkServer(srv *gorums.Server, impl BenchmarkServer) { - srv.RegisterHandler("benchmark.Benchmark.StartServerBenchmark", func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { - req := gorums.AsProto[*StartRequest](in) - resp, err := impl.StartServerBenchmark(ctx, req) - if err != nil { - return nil, err - } - return gorums.NewResponseMessage(in, resp), nil - }) - srv.RegisterHandler("benchmark.Benchmark.StopServerBenchmark", func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { - req := gorums.AsProto[*StopRequest](in) - resp, err := impl.StopServerBenchmark(ctx, req) - if err != nil { - return nil, err - } - return gorums.NewResponseMessage(in, resp), nil - }) - srv.RegisterHandler("benchmark.Benchmark.StartBenchmark", func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { - req := gorums.AsProto[*StartRequest](in) - resp, err := impl.StartBenchmark(ctx, req) - if err != nil { - return nil, err - } - return gorums.NewResponseMessage(in, resp), nil - }) - srv.RegisterHandler("benchmark.Benchmark.StopBenchmark", func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { - req := gorums.AsProto[*StopRequest](in) - resp, err := impl.StopBenchmark(ctx, req) - if err != nil { - return nil, err - } - return gorums.NewResponseMessage(in, resp), nil - }) - srv.RegisterHandler("benchmark.Benchmark.QuorumCall", func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { - req := gorums.AsProto[*Echo](in) - resp, err := impl.QuorumCall(ctx, req) - if err != nil { - return nil, err - } - return gorums.NewResponseMessage(in, resp), nil - }) - srv.RegisterHandler("benchmark.Benchmark.SlowServer", func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { - req := gorums.AsProto[*Echo](in) - resp, err := impl.SlowServer(ctx, req) - if err != nil { - return nil, err - } - return gorums.NewResponseMessage(in, resp), nil - }) - srv.RegisterHandler("benchmark.Benchmark.Multicast", func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { - req := gorums.AsProto[*TimedMsg](in) - impl.Multicast(ctx, req) - return nil, nil - }) -} diff --git a/benchmark/qspec.go b/benchmark/qspec.go deleted file mode 100644 index 76de3b509..000000000 --- a/benchmark/qspec.go +++ /dev/null @@ -1,37 +0,0 @@ -package benchmark - -import ( - "github.com/relab/gorums" -) - -// StopServerBenchmarkQF aggregates StopServerBenchmark responses from all nodes. -// It combines results, calculating averages and pooled variance. -func StopServerBenchmarkQF(replies map[uint32]*Result) (*Result, error) { - if len(replies) == 0 { - return nil, gorums.ErrIncomplete - } - // combine results, calculating averages and pooled variance - resp := &Result{} - for _, reply := range replies { - if resp.GetName() != "" { - resp.SetName(reply.GetName()) - } - resp.SetTotalOps(resp.GetTotalOps() + reply.GetTotalOps()) - resp.SetTotalTime(resp.GetTotalTime() + reply.GetTotalTime()) - resp.SetThroughput(resp.GetThroughput() + reply.GetThroughput()) - resp.SetLatencyAvg(resp.GetLatencyAvg() + reply.GetLatencyAvg()*float64(reply.GetTotalOps())) - resp.SetServerStats(append(resp.GetServerStats(), MemoryStat_builder{ - Allocs: reply.GetAllocsPerOp() * resp.GetTotalOps(), - Memory: reply.GetMemPerOp() * resp.GetTotalOps(), - }.Build())) - } - resp.SetLatencyAvg(resp.GetLatencyAvg() / float64(resp.GetTotalOps())) - for _, reply := range replies { - resp.SetLatencyVar(resp.GetLatencyVar() + float64(reply.GetTotalOps()-1)*reply.GetLatencyVar()) - } - resp.SetLatencyVar(resp.GetLatencyVar() / (float64(resp.GetTotalOps()) - float64(len(replies)))) - resp.SetTotalOps(resp.GetTotalOps() / uint64(len(replies))) - resp.SetTotalTime(resp.GetTotalTime() / int64(len(replies))) - resp.SetThroughput(resp.GetThroughput() / float64(len(replies))) - return resp, nil -} diff --git a/benchmark/server.go b/benchmark/server.go deleted file mode 100644 index 72145911e..000000000 --- a/benchmark/server.go +++ /dev/null @@ -1,101 +0,0 @@ -package benchmark - -import ( - context "context" - fmt "fmt" - log "log" - net "net" - "time" - - gorums "github.com/relab/gorums" -) - -type server struct { - stats *Stats -} - -func (srv *server) QuorumCall(_ gorums.ServerCtx, in *Echo) (resp *Echo, err error) { - return in, nil -} - -func (srv *server) AsyncQuorumCall(_ gorums.ServerCtx, in *Echo) (resp *Echo, err error) { - return in, nil -} - -func (srv *server) SlowServer(ctx gorums.ServerCtx, in *Echo) (resp *Echo, err error) { - ctx.Release() - time.Sleep(10 * time.Millisecond) - return in, nil -} - -func (srv *server) Multicast(_ gorums.ServerCtx, msg *TimedMsg) { - latency := time.Now().UnixNano() - msg.GetSendTime() - srv.stats.AddLatency(time.Duration(latency)) -} - -func (srv *server) StartServerBenchmark(_ gorums.ServerCtx, _ *StartRequest) (resp *StartResponse, err error) { - srv.stats.Clear() - srv.stats.Start() - return &StartResponse{}, nil -} - -func (srv *server) StopServerBenchmark(_ gorums.ServerCtx, _ *StopRequest) (resp *Result, err error) { - srv.stats.End() - return srv.stats.GetResult(), nil -} - -func (srv *server) StartBenchmark(_ gorums.ServerCtx, _ *StartRequest) (resp *StartResponse, err error) { - srv.stats.Clear() - srv.stats.Start() - return &StartResponse{}, nil -} - -func (srv *server) StopBenchmark(_ gorums.ServerCtx, _ *StopRequest) (resp *MemoryStat, err error) { - srv.stats.End() - return MemoryStat_builder{ - Allocs: srv.stats.endMs.Mallocs - srv.stats.startMs.Mallocs, - Memory: srv.stats.endMs.TotalAlloc - srv.stats.startMs.TotalAlloc, - }.Build(), nil -} - -// Server is a unified server for both ordered and unordered methods -type Server struct { - *gorums.Server - server server - stats Stats -} - -// NewBenchServer returns a new benchmark server -func NewBenchServer(opts ...gorums.ServerOption) *Server { - srv := &Server{} - srv.server.stats = &srv.stats - - srv.Server = gorums.NewServer(opts...) - RegisterBenchmarkServer(srv.Server, &srv.server) - return srv -} - -// StartLocalServers starts benchmark servers locally -func StartLocalServers(ctx context.Context, n int, opts ...gorums.ServerOption) []string { - var ports []string - basePort := 40000 - var servers []*Server - for p := basePort; p < basePort+n; p++ { - port := fmt.Sprintf(":%d", p) - ports = append(ports, port) - lis, err := net.Listen("tcp", port) - if err != nil { - log.Fatalf("Failed to start local server: %v\n", err) - } - srv := NewBenchServer(opts...) - servers = append(servers, srv) - go func() { _ = srv.Serve(lis) }() - } - go func() { - <-ctx.Done() - for _, srv := range servers { - srv.Stop() - } - }() - return ports -} diff --git a/benchmark/stats.go b/benchmark/stats.go deleted file mode 100644 index c7a31e176..000000000 --- a/benchmark/stats.go +++ /dev/null @@ -1,96 +0,0 @@ -package benchmark - -import ( - fmt "fmt" - math "math" - "runtime" - "strings" - "sync" - "time" -) - -// Format returns a tab formatted string representation of the result -func (r *Result) Format() string { - b := new(strings.Builder) - fmt.Fprintf(b, "%s\t", r.GetName()) - fmt.Fprintf(b, "%.2f ops/sec\t", r.GetThroughput()) - fmt.Fprintf(b, "%.2f ms\t", r.GetLatencyAvg()/float64(time.Millisecond)) - fmt.Fprintf(b, "%.2f ms\t", math.Sqrt(r.GetLatencyVar())/float64(time.Millisecond)) - fmt.Fprintf(b, "%d B/op\t", r.GetMemPerOp()) - fmt.Fprintf(b, "%d allocs/op\t", r.GetAllocsPerOp()) - return b.String() -} - -// Stats records and processes the raw data of a benchmark -type Stats struct { - mut sync.Mutex - startTime time.Time - endTime time.Time - startMs runtime.MemStats - endMs runtime.MemStats - - count uint64 - mean, m2 float64 -} - -// Start records the start time and memory stats -func (s *Stats) Start() { - s.mut.Lock() - defer s.mut.Unlock() - - runtime.ReadMemStats(&s.startMs) - s.startTime = time.Now() -} - -// End records the end time and memory stats -func (s *Stats) End() { - s.mut.Lock() - defer s.mut.Unlock() - - s.endTime = time.Now() - runtime.ReadMemStats(&s.endMs) -} - -// AddLatency adds a latency measurement -func (s *Stats) AddLatency(l time.Duration) { - s.mut.Lock() - defer s.mut.Unlock() - - // implements Welford's algorithm - s.count++ - delta := float64(l) - s.mean - s.mean += delta / float64(s.count) - delta2 := float64(l) - s.mean - s.m2 += delta * delta2 -} - -// GetResult computes and returns the result of the benchmark -func (s *Stats) GetResult() *Result { - s.mut.Lock() - defer s.mut.Unlock() - - r := &Result{} - r.SetTotalOps(s.count) - r.SetTotalTime(int64(s.endTime.Sub(s.startTime))) - r.SetThroughput(float64(r.GetTotalOps()) / float64(time.Duration(r.GetTotalTime()).Seconds())) - r.SetLatencyAvg(s.mean) - if s.count > 2 { - r.SetLatencyVar(s.m2 / float64(s.count-1)) - } - r.SetAllocsPerOp((s.endMs.Mallocs - s.startMs.Mallocs) / r.GetTotalOps()) - r.SetMemPerOp((s.endMs.TotalAlloc - s.startMs.TotalAlloc) / r.GetTotalOps()) - return r -} - -// Clear zeroes out the stats -func (s *Stats) Clear() { - s.mut.Lock() - s.startTime = time.Time{} - s.endTime = time.Time{} - s.startMs = runtime.MemStats{} - s.endMs = runtime.MemStats{} - s.count = 0 - s.mean = 0 - s.m2 = 0 - s.mut.Unlock() -} diff --git a/callopts_test.go b/callopts_test.go index b7b4b9916..b8cec261b 100644 --- a/callopts_test.go +++ b/callopts_test.go @@ -22,7 +22,9 @@ func testSystems(t testing.TB, n int) []*System { if _, ok := t.(*testing.B); !ok { t.Cleanup(func() { goleak.VerifyNone(t) }) } - systems, stop, err := NewLocalSystems(n, WithDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials()))) + systems, stop, err := NewLocalSystems(n, WithLocalDialOptions( + WithDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), + )) if err != nil { t.Fatal(err) } @@ -86,7 +88,7 @@ func TestCallOptionsIgnoreErrorsResourceLeak(t *testing.T) { }) } for _, sys := range systems { - sys.WaitForConfig(t.Context(), func(cfg Configuration) bool { + sys.WaitForPeers(t.Context(), func(cfg Configuration) bool { return cfg.Size() == 3 }) } diff --git a/cmd/benchmark/main.go b/cmd/benchmark/main.go deleted file mode 100644 index 3f3aa53e7..000000000 --- a/cmd/benchmark/main.go +++ /dev/null @@ -1,215 +0,0 @@ -package main - -import ( - "context" - "flag" - "fmt" - "net" - "os" - "os/signal" - "regexp" - "strings" - "syscall" - "text/tabwriter" - "time" - - "github.com/relab/gorums" - "github.com/relab/gorums/benchmark" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials/insecure" -) - -type regexpFlag struct { - val *regexp.Regexp -} - -func (f *regexpFlag) String() string { - if f.val == nil { - return "" - } - return fmt.Sprintf("'%s'", f.val.String()) -} - -func (f *regexpFlag) Set(v string) (err error) { - f.val, err = regexp.Compile(v) - return -} - -func (f *regexpFlag) Get() *regexp.Regexp { - return f.val -} - -type listFlag struct { - val []string -} - -func (f *listFlag) String() string { - return strings.Join(f.val, ",") -} - -func (f *listFlag) Set(v string) error { - f.val = strings.Split(v, ",") - return nil -} - -func (f *listFlag) Get() []string { - return f.val -} - -func listBenchmarks() { - tw := tabwriter.NewWriter(os.Stdout, 0, 0, 4, ' ', 0) - benchmarks := benchmark.GetBenchmarks(nil) - for _, b := range benchmarks { - fmt.Fprintf(tw, "%s:\t%s\n", b.Name, b.Description) - } - tw.Flush() -} - -func runServer(server string, recvSize, sendSize uint) { - signals := make(chan os.Signal, 1) - signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM) - - lis, err := net.Listen("tcp", server) - checkf("Failed to listen on '%s': %v", server, err) - - srv := benchmark.NewBenchServer(gorums.WithBufferSizes(recvSize, sendSize)) - go func() { checkf("serve failed: %v", srv.Serve(lis)) }() - - fmt.Printf("Running benchmark server on '%s'\n", server) - - <-signals - srv.Stop() -} - -func printResults(results []*benchmark.Result, options benchmark.Options, serverStats bool) { - resultWriter := tabwriter.NewWriter(os.Stdout, 0, 0, 4, ' ', 0) - fmt.Fprint(resultWriter, "Benchmark\tThroughput\tLatency\tStd.dev\tClient") - if !serverStats || !options.Remote { - fmt.Fprint(resultWriter, "+Servers\t\t") - } else if serverStats { - fmt.Fprint(resultWriter, "\t\t") - for i := 1; i <= options.NumNodes; i++ { - fmt.Fprintf(resultWriter, "Server %d\t\t", i) - } - } - fmt.Fprintln(resultWriter) - for _, r := range results { - if !serverStats && options.Remote { - for _, s := range r.GetServerStats() { - r.SetMemPerOp(r.GetMemPerOp() + s.GetMemory()/r.GetTotalOps()) - r.SetAllocsPerOp(r.GetAllocsPerOp() + s.GetAllocs()/r.GetTotalOps()) - } - } - fmt.Fprint(resultWriter, r.Format()) - if serverStats && options.Remote { - for _, s := range r.GetServerStats() { - fmt.Fprintf(resultWriter, "%d B/op\t%d allocs/op\t", s.GetMemory()/r.GetTotalOps(), s.GetAllocs()/r.GetTotalOps()) - } - } - fmt.Fprintln(resultWriter) - } - resultWriter.Flush() -} - -func main() { - var ( - benchmarksFlag = regexpFlag{val: regexp.MustCompile(".*")} - remotesFlag = listFlag{} - warmupFlag = flag.Duration("warmup", 100*time.Millisecond, "Warmup duration.") - benchTimeFlag = flag.Duration("time", 1*time.Second, "The duration of each benchmark.") - traceFile = flag.String("trace", "", "A `file` to write trace to.") - cpuprofile = flag.String("cpuprofile", "", "A `file` to write cpu profile to.") - memprofile = flag.String("memprofile", "", "A `file` to write memory profile to.") - payload = flag.Int("payload", 0, "Size of the payload in request and response messages (in bytes).") - concurrent = flag.Int("concurrent", 1, "Number of goroutines that can make calls concurrently.") - maxAsync = flag.Int("max-async", 1000, "Maximum number of async calls that can be in flight at once.") - server = flag.String("server", "", "Run a benchmark server on given `address`.") - serverStats = flag.Bool("server-stats", false, "Show server statistics separately") - cfgSize = flag.Int("config-size", 4, "Size of the configuration to use. If < 1, all nodes will be used.") - qSize = flag.Int("quorum-size", 0, "Number of replies to wait for before completing a quorum call.") - sendBuffer = flag.Uint("send-buffer", 0, "The size of the client's (and server's reverse channel) send buffer.") - recvBuffer = flag.Uint("recv-buffer", 0, "The size of the server's receive buffer.") - list = flag.Bool("list", false, "List all available benchmarks") - ) - flag.Var(&benchmarksFlag, "benchmarks", "A `regexp` matching the benchmarks to run.") - flag.Var(&remotesFlag, "remotes", "A comma separated `list` of remote addresses to connect to.") - flag.Parse() - - benchReg := benchmarksFlag.Get() - remotes := remotesFlag.Get() - - if *list { - listBenchmarks() - return - } - - stopProfilers, err := StartProfilers(*cpuprofile, *memprofile, *traceFile) - checkf("Failed to start profiling: %v", err) - defer func() { - checkf("Failed to stop profiling: %v", stopProfilers()) - }() - - if *server != "" { - runServer(*server, *recvBuffer, *sendBuffer) - return - } - - var options benchmark.Options - options.Concurrent = *concurrent - options.MaxAsync = *maxAsync - options.Payload = *payload - options.Warmup = *warmupFlag - options.Duration = *benchTimeFlag - options.Remote = true - - // start local servers if needed - if len(remotes) < 1 { - options.Remote = false - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - remotes = benchmark.StartLocalServers(ctx, *cfgSize, gorums.WithBufferSizes(*recvBuffer, *sendBuffer)) - } - - numNodes := len(remotes) - if *cfgSize < 1 || *cfgSize > numNodes { - options.NumNodes = numNodes - } else { - options.NumNodes = *cfgSize - } - - // find a valid value for QuorumSize - switch { - case options.NumNodes == 1: - options.QuorumSize = 1 - case *qSize < 1: - options.QuorumSize = options.NumNodes / 2 // the default value - case *qSize > options.NumNodes: - options.QuorumSize = options.NumNodes - default: - options.QuorumSize = *qSize - } - - dialOpts := []gorums.DialOption{ - gorums.WithDialOptions( - grpc.WithTransportCredentials(insecure.NewCredentials()), - ), - gorums.WithSendBufferSize(*sendBuffer), - } - cfg, err := gorums.NewConfig(gorums.WithNodeList(remotes[:options.NumNodes]), dialOpts...) - checkf("Failed to create configuration: %v", err) - defer cfg.Close() - - results, err := benchmark.RunBenchmarks(benchReg, options, cfg) - checkf("Error running benchmarks: %v", err) - - printResults(results, options, *serverStats) -} - -func checkf(format string, args ...any) { - for _, arg := range args { - if err, _ := arg.(error); err != nil { - fmt.Fprintf(os.Stderr, format, args...) - os.Exit(1) // skipcq: RVV-A0003 - } - } -} diff --git a/cmd/benchmark/profiling.go b/cmd/benchmark/profiling.go deleted file mode 100644 index 7f8b92520..000000000 --- a/cmd/benchmark/profiling.go +++ /dev/null @@ -1,101 +0,0 @@ -package main - -import ( - "os" - "runtime" - "runtime/pprof" - "runtime/trace" -) - -// StartCPUProfile starts a CPU profile that will be written to the given path. -// Returns a function to stop the profiler. -func StartCPUProfile(cpuProfilePath string) (stop func() error, err error) { - cpuProfile, err := os.Create(cpuProfilePath) - if err != nil { - return nil, err - } - if err := pprof.StartCPUProfile(cpuProfile); err != nil { - return nil, err - } - return func() error { - pprof.StopCPUProfile() - err = cpuProfile.Close() - if err != nil { - return err - } - return nil - }, nil -} - -// WriteMemProfile writes a memory profile to the given path. -func WriteMemProfile(memProfilePath string) error { - f, err := os.Create(memProfilePath) - if err != nil { - return err - } - runtime.GC() // get up-to-date statistics - if err := pprof.WriteHeapProfile(f); err != nil { - return err - } - err = f.Close() - return err -} - -// StartTrace starts a program trace using the "runtime/trace" package. -// Returns a function to stop the trace. -func StartTrace(tracePath string) (stop func() error, err error) { - traceFile, err := os.Create(tracePath) - if err != nil { - return nil, err - } - if err := trace.Start(traceFile); err != nil { - return nil, err - } - return func() error { - trace.Stop() - err = traceFile.Close() - if err != nil { - return err - } - return nil - }, nil -} - -// StartProfilers starts various profilers and returns a function to stop them. -func StartProfilers(cpuProfilePath, memProfilePath, tracePath string) (stopProfile func() error, err error) { - nilFunc := func() error { return nil } - - var ( - cpuProfileStop = nilFunc - traceStop = nilFunc - ) - - if cpuProfilePath != "" { - cpuProfileStop, err = StartCPUProfile(cpuProfilePath) - if err != nil { - return nil, err - } - } - - if tracePath != "" { - traceStop, err = StartTrace(tracePath) - if err != nil { - return nil, err - } - } - - return func() error { - err := cpuProfileStop() - if err != nil { - return err - } - err = traceStop() - if err != nil { - return err - } - if memProfilePath != "" { - err = WriteMemProfile(memProfilePath) - } - return err - }, nil -} diff --git a/doc/benchmarking.md b/doc/benchmarking.md deleted file mode 100644 index 45cefc5a2..000000000 --- a/doc/benchmarking.md +++ /dev/null @@ -1,96 +0,0 @@ -# Benchmarking Gorums - -The repository includes a program that can be used to benchmark different call types and options. -The program is compiled from `cmd/benchmark`, and uses the `benchmark` package. -Using the `benchmark` program, it is possible to run benchmarks on both local and remote servers. - -## Usage - -To compile the benchmark program, run: - -```shell -make benchmark -``` - -By default, the program runs all of the built-in benchmarks on local servers. -The following command-line flags can be used to change various parameters of the benchmarks: - -```text -Usage of cmd/benchmark/benchmark: - -benchmarks regexp - A regexp matching the benchmarks to run. (default '.*') - -concurrent int - Number of goroutines that can make calls concurrently. (default 1) - -config-size int - Size of the configuration to use. If < 1, all nodes will be used. (default 4) - -cpuprofile file - A file to write cpu profile to. - -list - List all available benchmarks - -max-async int - Maximum number of async calls that can be in flight at once. (default 1000) - -memprofile file - A file to write memory profile to. - -payload int - Size of the payload in request and response messages (in bytes). - -quorum-size int - Number of replies to wait for before completing a quorum call. - -remotes list - A comma separated list of remote addresses to connect to. - -send-buffer uint - The size of the send buffer. - -server address - Run a benchmark server on given address. - -server-buffer uint - The size of the server buffers. - -server-stats - Show server statistics separately - -time duration - The duration of each benchmark. (default "1s") - -trace file - A file to write trace to. - -warmup duration - Warmup duration. (default "100ms") -``` - -By default, the `cmd/benchmark` program starts internal servers to perform benchmarks locally. -To run the benchmarks with remote servers, the `--remotes` flag must be used. - -### Remote benchmarks with ansible - -In the `scripts/` folder, we provide some simple ansible scripts that can be used to run benchmarks on remote servers. -To use the scripts, an [inventory](https://docs.ansible.com/ansible/latest/user_guide/intro_inventory.html) file must be created. -The ansible script expects two groups, "client" and "servers." -Below is an example inventory file: - -```ini -[client] -client.example.com - -[servers] -server1.example.com -server2.example.com -server3.example.com -``` - -To copy the benchmark binary to the remote servers, run the `deploy.yml` ansible script as follows - -```sh -ansible-playbook -i [your inventory file] deploy.yml -``` - -(remember to build it using `make benchmark` first). - -The `benchmark.sh` script runs the appropriate ansible-playbook command and parses the output. -Hence, to run `benchmark.sh`: - -```sh -cd scripts/ -./benchmark.sh [your inventory file] [arguments to benchmark] -``` - -For example: - -```sh -./benchmark.sh ./hosts --benchmarks 'QC' -``` diff --git a/doc/dev-guide.md b/doc/dev-guide.md index 3f20271eb..45042aa1d 100644 --- a/doc/dev-guide.md +++ b/doc/dev-guide.md @@ -81,10 +81,6 @@ Or directly: go test -tags=integration ./... ``` -## Benchmarking - -See [benchmarking.md](./benchmarking.md) - ## Makefile Below is a description of the current `Makefile` targets. @@ -92,10 +88,9 @@ The `Makefile` itself also serves as documentation; inspect it for details. | Target | Description | | ----------------- | ------------------------------------------------------------------------------------------------------ | -| `all` | Builds `dev`, `benchmark`, and compiles tests (default target). | +| `all` | Builds `dev` and compiles tests (default target). | | `dev` | Updates `template_static.go` and regenerates generated files from templates. | -| `genproto` | Force-regenerates all protobuf and Gorums files across the repo (dev, benchmark, tests, examples). | -| `benchmark` | Compiles the benchmark tool. | +| `genproto` | Force-regenerates all protobuf and Gorums files across the repo (dev, tests, examples). | | `compiletests` | Compiles test protos in `internal/tests`. | | `tools` | Installs required tools (`protoc-gen-go`, `protoc-gen-go-grpc`, `stress`, etc.) via `go install tool`. | | `installgorums` | Reinstalls the `protoc-gen-gorums` plugin. | diff --git a/doc/user-guide.md b/doc/user-guide.md index 622cd6a0a..30ed4ea31 100644 --- a/doc/user-guide.md +++ b/doc/user-guide.md @@ -1168,26 +1168,25 @@ config, err := gorums.NewConfig( ) ``` -### WithConfig onChange Callback +### WithPeerChange Callback -`WithConfig` accepts an optional `onChange func(gorums.Configuration)` variadic argument. -The callback is called after every change to the known-peer configuration — that is, each time a pre-configured peer connects or disconnects. +`WithPeerChange` registers a callback invoked after every change to the connected-peer configuration — that is, each time a configured peer becomes reachable or unreachable. **Signature:** ```go -gorums.WithConfig(myNodeID, nodeListOption, func(cfg gorums.Configuration) { ... }) +gorums.WithPeerChange(func(cfg gorums.Configuration) { ... }) ``` -**When it runs:** after every change to the known-peer configuration. +**When it runs:** after every change to the connected-peer configuration. For peer connect/disconnect events, the callback runs inside the server's internal configuration lock, immediately after the configuration slice has been replaced. -The callback also fires once during `NewServer` construction (with the initial configuration, which contains only the self-node when no peers have connected yet)); that initial construction-time call does **not** run under the internal configuration lock. +The callback also fires once during `NewServer` construction, with the initial configuration, which contains only the self-node when no peers have connected yet; that initial construction-time call does **not** run under the internal configuration lock. -**What is available:** a configuration of connected known peers, sorted by node ID. +**What is available:** the connected-peer configuration, sorted by node ID. The self-node (this server's own ID) is always included regardless of connectivity. **Safe side effects:** signaling a channel, writing to an atomic, or copying the slice. -For connect/disconnect-triggered callbacks, the callback is invoked while holding the internal lock, so it must **not** call `srv.Config()`, `ctx.Config()`, or any other method that acquires the same lock. +For connect/disconnect-triggered callbacks, the callback is invoked while holding the internal lock, so it must **not** call `srv.ConnectedPeers()`, `ctx.ConnectedPeers()`, or any other method that acquires the same lock. Do not perform blocking or long-running work inside the callback, including during the initial construction-time call. #### Example: Reacting to Peer Membership Changes @@ -1200,16 +1199,15 @@ const quorumSize = 2 // majority for a three-node cluster, including self ready := make(chan struct{}, 1) gorumsSrv := gorums.NewServer( - gorums.WithConfig(myNodeID, gorums.WithNodeList(peerAddrs), - func(cfg gorums.Configuration) { - if len(cfg) >= quorumSize { - select { - case ready <- struct{}{}: - default: - } + gorums.WithPeers(myNodeID, gorums.WithNodeList(peerAddrs), dialOpts...), + gorums.WithPeerChange(func(cfg gorums.Configuration) { + if len(cfg) >= quorumSize { + select { + case ready <- struct{}{}: + default: } - }, - ), + } + }), ) // Block until a quorum connects before accepting client requests. @@ -1221,14 +1219,14 @@ The self-node is always present in `cfg`, so a three-node cluster (`quorumSize = ## Waiting for Configuration -`System.WaitForConfig` and `System.WaitForClientConfig` block until a condition on the configuration is satisfied, or until the context is cancelled or the system is stopped. -They replace the need to poll `Config()` in a loop and eliminate the latency and CPU overhead of polling. +`Server.WaitForPeers` and `Server.WaitForClients` block until a condition on the configuration is satisfied, or until the context is cancelled or the server is stopped. +They replace the need to poll `ConnectedPeers()` in a loop and eliminate the latency and CPU overhead of polling. ```go // Block until all three known peers are connected. ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() -if err := sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { +if err := srv.WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == 3 }); err != nil { log.Fatal("peers did not connect in time:", err) @@ -1237,24 +1235,24 @@ if err := sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { The condition is checked immediately against the current configuration, so the call returns without blocking if the condition is already satisfied. -### WaitForConfig +### WaitForPeers -`WaitForConfig` waits on the known-peer configuration — the set of pre-configured peers that have connected, plus the local node itself. +`WaitForPeers` waits on the connected-peer configuration — the subset of the configured peers this server can currently reach, plus the local node itself (see `ConnectedPeers()`). Use this when you need a quorum of static cluster members to be present before beginning to serve requests. ```go -err := sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { +err := srv.WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() >= quorumSize }) ``` -### WaitForClientConfig +### WaitForClients -`WaitForClientConfig` waits on the client-peer configuration — the set of anonymous clients that have connected dynamically and are reachable for reverse-direction calls. +`WaitForClients` waits on the client-peer configuration — the set of anonymous clients that have connected dynamically and are reachable for reverse-direction calls. Use this when a server should not proceed until a minimum number of clients have registered. ```go -err := sys.WaitForClientConfig(ctx, func(cfg gorums.Configuration) bool { +err := srv.WaitForClients(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() >= expectedClients }) ``` @@ -1265,14 +1263,14 @@ err := sys.WaitForClientConfig(ctx, func(cfg gorums.Configuration) bool { | --------------------------------- | ------------------- | | `cond` returns `true` | `nil` | | `ctx` is cancelled or times out | `ctx.Err()` | -| `sys.Stop()` called before `cond` | `gorums.ErrStopped` | +| `srv.Stop()` called before `cond` | `gorums.ErrStopped` | ### Relationship to `onChange` -`WaitForConfig` and the `onChange` callback (see [WithConfig onChange Callback](#withconfig-onchange-callback)) serve complementary purposes. +`WaitForPeers` and the `WithPeerChange` callback (see [WithPeerChange Callback](#withpeerchange-callback)) serve complementary purposes. `onChange` is suited for reactive work that must happen synchronously on every configuration change — for example, triggering a leader election or updating an atomic counter. -`WaitForConfig` is suited for startup synchronization — blocking until the cluster reaches a desired state before the application begins normal operation. -Unlike `onChange`, `WaitForConfig` composes naturally with `context.WithTimeout` and `context.WithCancel`. +`WaitForPeers` is suited for startup synchronization — blocking until the cluster reaches a desired state before the application begins normal operation. +Unlike the callback, `WaitForPeers` composes naturally with `context.WithTimeout` and `context.WithCancel`. ## Error Handling @@ -1750,13 +1748,14 @@ greeting = gorums The `nread` and `nwrite` commands trigger server-side nested quorum calls and nested multicasts, which are described in the following sections. -## Nested Quorum Calls with ServerCtx.Config +## Nested Quorum Calls with ServerCtx.PeerConfig A server handler (the server method itself) can act as a client and issue its own quorum calls to other nodes. These are called *nested quorum calls*, because one quorum call triggers another from inside the server handler. -`ServerCtx.Config()` returns a `Configuration` of all currently connected known peers, as configured with `gorums.WithConfig`. +`ServerCtx.PeerConfig()` returns the `Configuration` of the peers the server was configured with via `gorums.WithPeers`. This makes it straightforward for a handler to fan out a sub-request to the rest of the cluster. +It is the full peer set, not the reachable subset, so a quorum size derived from it inside a handler does not shift as peers connect and disconnect; use `ctx.ConnectedPeers()` to observe reachability. ### Setting Up Peer Tracking @@ -1764,20 +1763,23 @@ Enable peer tracking for the server at construction time: ```go gorumsSrv := gorums.NewServer( - gorums.WithConfig(myNodeID, gorums.WithNodeList(peerAddrs)), + gorums.WithPeers(myNodeID, gorums.WithNodeList(peerAddrs), dialOpts...), ) ``` The `myNodeID` is this server's own node ID. -It is included in the configuration returned by `Config()` so that all quorum thresholds account for the local replica. +It is included in the configuration returned by `PeerConfig()` so that all quorum thresholds account for the local replica; calls to the local node are served in-process. -Each node in `peerAddrs` that connects sends its node ID in connection metadata. -When the peer connects, `Config()` starts returning that node as an available target. +Peers appear in `ConnectedPeers()` as connections to them are established. +Use `WaitForPeers` to wait for enough peers to connect before issuing calls. -The storage example uses `gorums.NewLocalSystems`, which calls `WithConfig` automatically for each system and stores the node list for outbound configuration. +A symmetric server re-establishes an outbound stream proactively when it drops while idle, rather than waiting for the next local send. +Without this, a peer would remain absent from the remote's `ConnectedPeers()` until that side happened to send something. -`WithConfig` also accepts an optional `onChange` callback that is called each time the peer configuration changes. -See [Server Configuration Callbacks](#server-configuration-callbacks) for details and an example. +The storage example uses `gorums.NewLocalSystems`, which calls `WithPeers` automatically for each system. + +Register a `WithPeerChange` callback to react each time the connected-peer configuration changes. +See [WithPeerChange Callback](#withpeerchange-callback) for details and an example. ### Writing the Handler @@ -1789,7 +1791,7 @@ Without `Release()`, the server would block all other inbound messages until the // ReadNestedQC is a quorum-call handler that fans out a nested ReadQC // to all known connected peers and returns the most recent value. func (s *storageServer) ReadNestedQC(ctx gorums.ServerCtx, req *pb.ReadRequest) (*pb.ReadResponse, error) { - config := ctx.Config() + config := ctx.PeerConfig() if len(config) == 0 { return nil, fmt.Errorf("read_nested_qc: requires a server peer configuration") } @@ -1804,7 +1806,7 @@ The same pattern applies to nested multicast: ```go func (s *storageServer) WriteNestedMulticast(ctx gorums.ServerCtx, req *pb.WriteRequest) (*pb.WriteResponse, error) { - config := ctx.Config() + config := ctx.PeerConfig() if len(config) == 0 { return nil, fmt.Errorf("write_nested_multicast: requires server peer configuration") } @@ -1839,18 +1841,18 @@ sequenceDiagram The client sees a single quorum call, but internally each receiving node fans out to all of its peers and returns the freshest value found across the whole cluster. -## Reverse Direction Calls with ServerCtx.ClientConfig +## Reverse Direction Calls with ServerCtx.ConnectedClients -`ServerCtx.ClientConfig()` returns a `Configuration` of all currently connected *client peers* — nodes that connected to this server dynamically, rather than being pre-configured with `WithConfig`. +`ServerCtx.ConnectedClients()` returns a `Configuration` of all currently connected *client peers* — nodes that connected to this server dynamically, rather than being pre-configured with `WithPeers`. A handler can use this configuration to make outbound calls back towards those clients, reversing the usual direction of communication. This pattern is particularly useful when clients are behind a firewall and cannot accept inbound connections. Clients can still initiate outbound connections to a server with a public IP address. -Once a client establishes a connection, the server retains a reverse-direction stream back to that client, and `ClientConfig()` includes it as a callable target. +Once a client establishes a connection, the server retains a reverse-direction stream back to that client, and `ConnectedClients()` includes it as a callable target. The server can therefore fan out quorum calls to all connected clients without requiring any additional network connections or firewall rules. A typical setup: each client connects to the server and calls a `Register` RPC to announce itself as ready. -When the server's handler is later invoked — for example, by an external coordinator — it uses `ctx.ClientConfig()` to fan out the call to all registered clients and aggregate their responses. +When the server's handler is later invoked — for example, by an external coordinator — it uses `ctx.ConnectedClients()` to fan out the call to all registered clients and aggregate their responses. ### Setting Up the Server @@ -1861,12 +1863,12 @@ No additional option is needed: gorumsSrv := gorums.NewServer() ``` -`ClientConfig()` is always available and reflects currently connected clients. -If you also need to track known peers with static node IDs, combine with `WithConfig` (mixed mode): +`ConnectedClients()` is always available and reflects currently connected clients. +If you also need to track known peers with static node IDs, combine with `WithPeers` (mixed mode): ```go gorumsSrv := gorums.NewServer( - gorums.WithConfig(myNodeID, gorums.WithNodeList(knownPeers)), // static known peers + gorums.WithPeers(myNodeID, gorums.WithNodeList(knownPeers), dialOpts...), // static known peers // anonymous clients are tracked automatically ) ``` @@ -1877,14 +1879,14 @@ For example, a local test cluster: systems, stop, err := gorums.NewLocalSystems(4) ``` -> **Note:** The `nread` and `nwrite` commands in the storage REPL example use `ctx.Config()` (the static server-to-server direction) rather than `ctx.ClientConfig()`. +> **Note:** The `nread` and `nwrite` commands in the storage REPL example use `ctx.PeerConfig()` (the static server-to-server direction) rather than `ctx.ConnectedClients()`. > Reverse direction calls require every participant to act as both a Gorums server *and* to expose its own server method handlers so that peers can call back to it. -> The REPL client in the storage example does not implement any server method handlers, so calling back to it via `ClientConfig()` is not supported in that example. +> The REPL client in the storage example does not implement any server method handlers, so calling back to it via `ConnectedClients()` is not supported in that example. ### Setting Up the Client For the server to call back to a client, the client must expose its own method handlers over the same bidirectional stream it opens to the server. -Create a `*gorums.Server`, register any handler methods the server may invoke, and then pass it to `WithServer` when establishing the outbound connection: +Create a `*gorums.Server`, register any handler methods the server may invoke, and then pass it to `WithBackChannel` when establishing the outbound connection: ```go // Create a server to host the client-side handlers. @@ -1893,25 +1895,25 @@ clientSrv := gorums.NewServer() // Register the methods the remote server is allowed to call back on this client. clientSrv.RegisterHandler(pb.MyMethod, myHandler) -// Connect to the server; WithServer wires up the back-channel dispatcher automatically. +// Connect to the server; WithBackChannel wires up the back-channel dispatcher automatically. config, err := gorums.NewConfig( gorums.WithNodeList(serverAddrs), - gorums.WithServer(clientSrv), + gorums.WithBackChannel(clientSrv), gorums.WithDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), ) ``` -Passing `clientSrv` to `gorums.WithServer` is what installs the server as the back-channel request handler. -When the remote server dispatches a reverse-direction call via `ctx.ClientConfig()`, the call arrives on the same gRPC stream the client opened and is routed to `clientSrv` for dispatch. +Passing `clientSrv` to `gorums.WithBackChannel` is what installs the server as the back-channel request handler. +When the remote server dispatches a reverse-direction call via `ctx.ConnectedClients()`, the call arrives on the same gRPC stream the client opened and is routed to `clientSrv` for dispatch. The client does **not** need to open a separate listening socket — the handler is served entirely over the existing outbound connection. ### Connecting as an Anonymous Client -A client must announce `NodeID=0` in its connection metadata to be assigned a dynamic node ID by the server and to appear in `ClientConfig()` for reverse-direction calls. +A client must announce `NodeID=0` in its connection metadata to be assigned a dynamic node ID by the server and to appear in `ConnectedClients()` for reverse-direction calls. Clients behind a firewall typically have no pre-configured node ID, so they connect as anonymous clients. -Connecting via `gorums.NewConfig(..., gorums.WithServer(clientSrv), ...)` without `WithConfig` sends `NodeID=0` automatically. -The server assigns it a dynamic ID and includes it in `ClientConfig()`. +Connecting via `gorums.NewConfig(..., gorums.WithBackChannel(clientSrv), ...)` without `WithPeers` sends `NodeID=0` automatically. +The server assigns it a dynamic ID and includes it in `ConnectedClients()`. The client can then call `Register` (a unicast) to signal that it is ready to receive calls: ```go @@ -1919,16 +1921,16 @@ nodeCtx := serverNode.Context(ctx) err = pb.Register(nodeCtx, &pb.RegisterRequest{}) ``` -In contrast, a client with a pre-configured node ID (created with `WithConfig`) announces its static node ID — servers with a matching node list will put it in their static `Config()`, not `ClientConfig()`. +In contrast, a client with a pre-configured node ID (created with `WithPeers`) announces its static node ID — servers with a matching node list will put it in their `ConnectedPeers()`, not `ConnectedClients()`. ### Writing the Handler -The handler reads `ctx.ClientConfig()` to reach all currently connected client peers: +The handler reads `ctx.ConnectedClients()` to reach all currently connected client peers: ```go // ReadNestedQC fans out a ReadQC to all clients that have connected. func (s *storageServer) ReadNestedQC(ctx gorums.ServerCtx, req *pb.ReadRequest) (*pb.ReadResponse, error) { - config := ctx.ClientConfig() + config := ctx.ConnectedClients() if len(config) == 0 { return nil, fmt.Errorf("read_nested_qc: no client peers connected") } @@ -1937,12 +1939,12 @@ func (s *storageServer) ReadNestedQC(ctx gorums.ServerCtx, req *pb.ReadRequest) } ``` -The key difference from `ServerCtx.Config()` is the direction of each per-node connection: +The key difference from `ServerCtx.PeerConfig()` is the direction of each per-node connection: | Method | Connection direction | Typical use case | | -------------------- | ----------------------------------------------- | -------------------------------------------------- | -| `ctx.Config()` | Outbound (this server connects to peers) | Static cluster with known membership | -| `ctx.ClientConfig()` | Inbound reversed (server calls back to clients) | Clients behind a firewall that connect to a server | +| `ctx.PeerConfig()` | Outbound (this server connects to peers) | Static cluster with known membership | +| `ctx.ConnectedClients()` | Inbound reversed (server calls back to clients) | Clients behind a firewall that connect to a server | ### Sequence Diagram @@ -1960,7 +1962,7 @@ sequenceDiagram C->>Srv: [connect + Register()] Coord->>Srv: ReadNestedQC(key) - Note over Srv: ctx.ClientConfig() = {A, B, C} + Note over Srv: ctx.ConnectedClients() = {A, B, C} Note over Srv: ctx.Release() Srv->>A: ReadQC(key) [reverse-direction] Srv->>B: ReadQC(key) [reverse-direction] @@ -1973,3 +1975,13 @@ sequenceDiagram ``` The reverse-direction calls reuse the existing inbound gRPC streams established by the clients, so no additional network connections or firewall rules are needed. + +## Send Queue Capacity and Backpressure + +The per-node send queue defaults to 4096 entries. +Passing zero to `WithSendBufferSize` or the send-size argument of `WithBufferSizes` selects that default. +Two-way requests fail fast with an unavailable error when a real buffered queue is full: a full queue means the peer is not draining sends, and failing fast lets quorum logic count that peer as failed instead of stalling every caller behind it. +One-way client calls (`Unicast`, `Multicast`) wait for space instead, since backpressure on the caller is what paces them. +A reply sent from a receive or dispatch loop — a server-initiated back-channel reply or a server-side inbound reply — also fails fast instead of waiting, or is silently dropped if it has no error channel to report on. +Waiting there could stall that connection from reading further messages. +Applications that previously relied on an unbuffered send queue should remove that assumption and choose an explicit positive capacity when a smaller backlog is required. diff --git a/errors.go b/errors.go index 704a5778a..8c2ddc9e3 100644 --- a/errors.go +++ b/errors.go @@ -18,12 +18,29 @@ var ErrSendFailure = errors.New("send failure") // ErrTypeMismatch is returned when a response cannot be cast to the expected type. var ErrTypeMismatch = stream.ErrTypeMismatch +// ErrStreamDown is returned for a call that cannot be delivered or retried +// because the target node's stream is unavailable. It is a gRPC status error +// with the Unavailable code; match its identity with [errors.Is], including +// against a node error inside a [QuorumCallError]. +var ErrStreamDown = stream.ErrStreamDown + +// ErrNodeClosed is returned for a call enqueued after its node was closed. It +// is a gRPC status error with the Unavailable code; match it with [errors.Is]. +var ErrNodeClosed = stream.ErrNodeClosed + +// ErrSendQueueFull is returned for a two-way call enqueued while the node's +// send queue is at capacity: a full queue means the peer is not draining sends, +// so the call fails fast (letting quorum logic count the peer as failed) rather +// than block behind it. It is a gRPC status error with the Unavailable code; +// match it with [errors.Is]. See [WithSendBufferSize] for the capacity. +var ErrSendQueueFull = stream.ErrSendQueueFull + // ErrSkipNode is returned when a node is skipped by request transformations. // This allows the response iterator to account for all nodes without blocking. var ErrSkipNode = errors.New("skip node") -// ErrStopped is returned by [System.WaitForConfig] and [System.WaitForClientConfig] -// when the system is stopped before the condition is met. +// ErrStopped is returned by [Server.WaitForPeers] and [Server.WaitForClients] +// when the server is stopped before the condition is met. var ErrStopped = errors.New("system stopped") // QuorumCallError reports on a failed quorum call. diff --git a/examples/storage/server.go b/examples/storage/server.go index b60e10f8b..7d845daf0 100644 --- a/examples/storage/server.go +++ b/examples/storage/server.go @@ -31,10 +31,10 @@ func runServer(address string, peers []string, srvOpt gorums.ServerOption) error if err != nil { return err } + insecureDial := gorums.WithDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())) sys, err := gorums.NewSystem(address, - gorums.WithServerOptions(srvOpt, gorums.WithConfig(myID, peerList)), - gorums.WithOutboundNodes(peerList), - gorums.WithDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), + gorums.WithServerOptions(srvOpt, gorums.WithPeers(myID, peerList, insecureDial)), + insecureDial, ) if err != nil { return fmt.Errorf("failed to create system on %q: %w", address, err) @@ -48,7 +48,7 @@ func runServer(address string, peers []string, srvOpt gorums.ServerOption) error ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - if err := sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + if err := sys.WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == len(peers) }); err != nil { return fmt.Errorf("peers did not connect in time: %w", err) @@ -65,7 +65,10 @@ func runServer(address string, peers []string, srvOpt gorums.ServerOption) error // call stop when the cluster is no longer needed. func runLocalCluster(srvOpts gorums.ServerOption) ([]string, func(), error) { dialOpts := gorums.WithDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())) - systems, stop, err := gorums.NewLocalSystems(4, gorums.WithServerOptions(srvOpts), dialOpts) + systems, stop, err := gorums.NewLocalSystems(4, + gorums.WithLocalServerOptions(srvOpts), + gorums.WithLocalDialOptions(dialOpts), + ) if err != nil { return nil, nil, fmt.Errorf("failed to create local systems: %w", err) } @@ -80,7 +83,7 @@ func runLocalCluster(srvOpts gorums.ServerOption) ([]string, func(), error) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() for _, sys := range systems { - if err := sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + if err := sys.WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == len(systems) }); err != nil { stop() @@ -214,9 +217,9 @@ func (s *storageServer) ReadCorrectable(_ gorums.ServerCtx, req *pb.ReadRequest, } // ReadNestedQC is a quorum-call handler that performs a nested quorum call -// using the server's known-peer configuration from WithConfig. +// using the server's peer configuration from WithPeers. func (s *storageServer) ReadNestedQC(ctx gorums.ServerCtx, req *pb.ReadRequest) (resp *pb.ReadResponse, err error) { - cfg := ctx.Config() + cfg := ctx.PeerConfig() if len(cfg) == 0 { return nil, fmt.Errorf("read_nested_qc: requires server peer configuration") } @@ -226,9 +229,9 @@ func (s *storageServer) ReadNestedQC(ctx gorums.ServerCtx, req *pb.ReadRequest) } // WriteNestedMulticast is a quorum-call handler that performs a nested multicast -// using the server's known-peer configuration from WithConfig. +// using the server's peer configuration from WithPeers. func (s *storageServer) WriteNestedMulticast(ctx gorums.ServerCtx, req *pb.WriteRequest) (resp *pb.WriteResponse, err error) { - cfg := ctx.Config() + cfg := ctx.PeerConfig() if len(cfg) == 0 { return nil, fmt.Errorf("write_nested_multicast: requires server peer configuration") } diff --git a/go.mod b/go.mod index e875fb1f5..ec71b6175 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,6 @@ go 1.26.0 require ( github.com/google/go-cmp v0.7.0 go.uber.org/goleak v1.3.0 - golang.org/x/sync v0.20.0 golang.org/x/tools v0.43.0 google.golang.org/genproto/googleapis/rpc v0.0.0-20260319201613-d00831a3d3e7 google.golang.org/grpc v1.79.3 @@ -16,6 +15,7 @@ require ( golang.org/x/exp v0.0.0-20260312153236-7ab1446f8b90 // indirect golang.org/x/mod v0.34.0 // indirect golang.org/x/net v0.52.0 // indirect + golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.42.0 // indirect golang.org/x/text v0.35.0 // indirect google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.6.1 // indirect diff --git a/go.sum b/go.sum index 3051890f5..93ffa670e 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= +github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= @@ -16,51 +18,30 @@ github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PK github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= -go.opentelemetry.io/otel v1.38.0 h1:RkfdswUDRimDg0m2Az18RKOsnI8UDzppJAtj01/Ymk8= -go.opentelemetry.io/otel v1.38.0/go.mod h1:zcmtmQ1+YmQM9wrNsTGV/q/uyusom3P8RxwExxkZhjM= go.opentelemetry.io/otel v1.39.0 h1:8yPrr/S0ND9QEfTfdP9V+SiwT4E0G7Y5MO7p85nis48= -go.opentelemetry.io/otel/metric v1.38.0 h1:Kl6lzIYGAh5M159u9NgiRkmoMKjvbsKtYRwgfrA6WpA= -go.opentelemetry.io/otel/metric v1.38.0/go.mod h1:kB5n/QoRM8YwmUahxvI3bO34eVtQf2i4utNVLr9gEmI= +go.opentelemetry.io/otel v1.39.0/go.mod h1:kLlFTywNWrFyEdH0oj2xK0bFYZtHRYUdv1NklR/tgc8= go.opentelemetry.io/otel/metric v1.39.0 h1:d1UzonvEZriVfpNKEVmHXbdf909uGTOQjA0HF0Ls5Q0= -go.opentelemetry.io/otel/sdk v1.38.0 h1:l48sr5YbNf2hpCUj/FoGhW9yDkl+Ma+LrVl8qaM5b+E= -go.opentelemetry.io/otel/sdk v1.38.0/go.mod h1:ghmNdGlVemJI3+ZB5iDEuk4bWA3GkTpW+DOoZMYBVVg= +go.opentelemetry.io/otel/metric v1.39.0/go.mod h1:jrZSWL33sD7bBxg1xjrqyDjnuzTUB0x1nBERXd7Ftcs= go.opentelemetry.io/otel/sdk v1.39.0 h1:nMLYcjVsvdui1B/4FRkwjzoRVsMK8uL/cj0OyhKzt18= -go.opentelemetry.io/otel/sdk/metric v1.38.0 h1:aSH66iL0aZqo//xXzQLYozmWrXxyFkBJ6qT5wthqPoM= -go.opentelemetry.io/otel/sdk/metric v1.38.0/go.mod h1:dg9PBnW9XdQ1Hd6ZnRz689CbtrUp0wMMs9iPcgT9EZA= +go.opentelemetry.io/otel/sdk v1.39.0/go.mod h1:vDojkC4/jsTJsE+kh+LXYQlbL8CgrEcwmt1ENZszdJE= go.opentelemetry.io/otel/sdk/metric v1.39.0 h1:cXMVVFVgsIf2YL6QkRF4Urbr/aMInf+2WKg+sEJTtB8= -go.opentelemetry.io/otel/trace v1.38.0 h1:Fxk5bKrDZJUH+AMyyIXGcFAPah0oRcT+LuNtJrmcNLE= -go.opentelemetry.io/otel/trace v1.38.0/go.mod h1:j1P9ivuFsTceSWe1oY+EeW3sc+Pp42sO++GHkg4wwhs= +go.opentelemetry.io/otel/sdk/metric v1.39.0/go.mod h1:xq9HEVH7qeX69/JnwEfp6fVq5wosJsY1mt4lLfYdVew= go.opentelemetry.io/otel/trace v1.39.0 h1:2d2vfpEDmCJ5zVYz7ijaJdOF59xLomrvj7bjt6/qCJI= +go.opentelemetry.io/otel/trace v1.39.0/go.mod h1:88w4/PnZSazkGzz/w84VHpQafiU4EtqqlVdxWy+rNOA= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= -golang.org/x/exp v0.0.0-20251219203646-944ab1f22d93 h1:fQsdNF2N+/YewlRZiricy4P1iimyPKZ/xwniHj8Q2a0= -golang.org/x/exp v0.0.0-20251219203646-944ab1f22d93/go.mod h1:EPRbTFwzwjXj9NpYyyrvenVh9Y+GFeEvMNh7Xuz7xgU= -golang.org/x/exp v0.0.0-20260218203240-3dfff04db8fa h1:Zt3DZoOFFYkKhDT3v7Lm9FDMEV06GpzjG2jrqW+QTE0= -golang.org/x/exp v0.0.0-20260218203240-3dfff04db8fa/go.mod h1:K79w1Vqn7PoiZn+TkNpx3BUWUQksGO3JcVX6qIjytmA= golang.org/x/exp v0.0.0-20260312153236-7ab1446f8b90 h1:jiDhWWeC7jfWqR9c/uplMOqJ0sbNlNWv0UkzE0vX1MA= golang.org/x/exp v0.0.0-20260312153236-7ab1446f8b90/go.mod h1:xE1HEv6b+1SCZ5/uscMRjUBKtIxworgEcEi+/n9NQDQ= -golang.org/x/mod v0.31.0 h1:HaW9xtz0+kOcWKwli0ZXy79Ix+UW/vOfmWI5QVd2tgI= -golang.org/x/mod v0.31.0/go.mod h1:43JraMp9cGx1Rx3AqioxrbrhNsLl2l/iNAvuBkrezpg= golang.org/x/mod v0.34.0 h1:xIHgNUUnW6sYkcM5Jleh05DvLOtwc6RitGHbDk4akRI= golang.org/x/mod v0.34.0/go.mod h1:ykgH52iCZe79kzLLMhyCUzhMci+nQj+0XkbXpNYtVjY= -golang.org/x/net v0.48.0 h1:zyQRTTrjc33Lhh0fBgT/H3oZq9WuvRR5gPC70xpDiQU= -golang.org/x/net v0.48.0/go.mod h1:+ndRgGjkh8FGtu1w1FGbEC31if4VrNVMuKTgcAAnQRY= golang.org/x/net v0.52.0 h1:He/TN1l0e4mmR3QqHMT2Xab3Aj3L9qjbhRm78/6jrW0= golang.org/x/net v0.52.0/go.mod h1:R1MAz7uMZxVMualyPXb+VaqGSa3LIaUqk0eEt3w36Sw= -golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= -golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= -golang.org/x/sys v0.39.0 h1:CvCKL8MeisomCi6qNZ+wbb0DN9E5AATixKsvNtMoMFk= -golang.org/x/sys v0.39.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= golang.org/x/sys v0.42.0 h1:omrd2nAlyT5ESRdCLYdm3+fMfNFE/+Rf4bDIQImRJeo= golang.org/x/sys v0.42.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/text v0.32.0 h1:ZD01bjUt1FQ9WJ0ClOL5vxgxOI/sVCNgX1YtKwcY0mU= -golang.org/x/text v0.32.0/go.mod h1:o/rUWzghvpD5TXrTIBuJU77MTaN0ljMWE47kxGJQ7jY= golang.org/x/text v0.35.0 h1:JOVx6vVDFokkpaq1AEptVzLTpDe9KGpj5tR4/X+ybL8= golang.org/x/text v0.35.0/go.mod h1:khi/HExzZJ2pGnjenulevKNX1W67CUy0AsXcNubPGCA= -golang.org/x/tools v0.40.0 h1:yLkxfA+Qnul4cs9QA3KnlFu0lVmd8JJfoq+E41uSutA= -golang.org/x/tools v0.40.0/go.mod h1:Ik/tzLRlbscWpqqMRjyWYDisX8bG13FrdXp3o4Sr9lc= golang.org/x/tools v0.43.0 h1:12BdW9CeB3Z+J/I/wj34VMl8X+fEXBxVR90JeMX5E7s= golang.org/x/tools v0.43.0/go.mod h1:uHkMso649BX2cZK6+RpuIPXS3ho2hZo4FVwfoy1vIk0= golang.org/x/tools/go/expect v0.1.1-deprecated h1:jpBZDwmgPhXsKZC6WhL20P4b/wmnpsEAGHaNy0n/rJM= @@ -69,20 +50,10 @@ golang.org/x/tools/go/packages/packagestest v0.1.1-deprecated h1:1h2MnaIAIXISqTF golang.org/x/tools/go/packages/packagestest v0.1.1-deprecated/go.mod h1:RVAQXBGNv1ib0J382/DPCRS/BPnsGebyM1Gj5VSDpG8= gonum.org/v1/gonum v0.16.0 h1:5+ul4Swaf3ESvrOnidPp4GZbzf0mxVQpDCYUQE7OJfk= gonum.org/v1/gonum v0.16.0/go.mod h1:fef3am4MQ93R2HHpKnLk4/Tbh/s0+wqD5nfa6Pnwy4E= -google.golang.org/genproto/googleapis/rpc v0.0.0-20251222181119-0a764e51fe1b h1:Mv8VFug0MP9e5vUxfBcE3vUkV6CImK3cMNMIDFjmzxU= -google.golang.org/genproto/googleapis/rpc v0.0.0-20251222181119-0a764e51fe1b/go.mod h1:j9x/tPzZkyxcgEFkiKEEGxfvyumM01BEtsW8xzOahRQ= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260311181403-84a4fc48630c h1:xgCzyF2LFIO/0X2UAoVRiXKU5Xg6VjToG4i2/ecSswk= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260311181403-84a4fc48630c/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= google.golang.org/genproto/googleapis/rpc v0.0.0-20260319201613-d00831a3d3e7 h1:ndE4FoJqsIceKP2oYSnUZqhTdYufCYYkqwtFzfrhI7w= google.golang.org/genproto/googleapis/rpc v0.0.0-20260319201613-d00831a3d3e7/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= -google.golang.org/grpc v1.78.0 h1:K1XZG/yGDJnzMdd/uZHAkVqJE+xIDOcmdSFZkBUicNc= -google.golang.org/grpc v1.78.0/go.mod h1:I47qjTo4OKbMkjA/aOOwxDIiPSBofUtQUI5EfpWvW7U= -google.golang.org/grpc v1.79.2 h1:fRMD94s2tITpyJGtBBn7MkMseNpOZU8ZxgC3MMBaXRU= -google.golang.org/grpc v1.79.2/go.mod h1:KmT0Kjez+0dde/v2j9vzwoAScgEPx/Bw1CYChhHLrHQ= google.golang.org/grpc v1.79.3 h1:sybAEdRIEtvcD68Gx7dmnwjZKlyfuc61Dyo9pGXXkKE= google.golang.org/grpc v1.79.3/go.mod h1:KmT0Kjez+0dde/v2j9vzwoAScgEPx/Bw1CYChhHLrHQ= -google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.6.0 h1:6Al3kEFFP9VJhRz3DID6quisgPnTeZVr4lep9kkxdPA= -google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.6.0/go.mod h1:QLvsjh0OIR0TYBeiu2bkWGTJBUNQ64st52iWj/yA93I= google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.6.1 h1:/WILD1UcXj/ujCxgoL/DvRgt2CP3txG8+FwkUbb9110= google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.6.1/go.mod h1:YNKnb2OAApgYn2oYY47Rn7alMr1zWjb2U8Q0aoGWiNc= google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= diff --git a/gorumstest/gorumstest.go b/gorumstest/gorumstest.go index ada0f63f1..6409b7f6c 100644 --- a/gorumstest/gorumstest.go +++ b/gorumstest/gorumstest.go @@ -228,11 +228,12 @@ func Servers(t testing.TB, numServers int, srvFn func(i int) gorums.ServerIface) } // Systems returns n started Gorums systems on random localhost ports (see -// [gorums.NewLocalSystems]). Each system auto-creates an outbound +// [gorums.NewLocalSystems]). Each system auto-creates a peer // [gorums.Configuration] over the group, accessible via // [gorums.System.OutboundConfig]. The systems are automatically stopped when -// the test finishes via t.Cleanup. -func Systems(t testing.TB, n int) []*gorums.System { +// the test finishes via t.Cleanup. Any [gorums.ServerOption]s are applied to +// every server. +func Systems(t testing.TB, n int, opts ...gorums.ServerOption) []*gorums.System { t.Helper() // Skip goleak check for benchmarks @@ -241,7 +242,10 @@ func Systems(t testing.TB, n int) []*gorums.System { t.Cleanup(func() { goleak.VerifyNone(t) }) } - systems, stop, err := gorums.NewLocalSystems(n, InsecureDialOptions(t)) + systems, stop, err := gorums.NewLocalSystems(n, + gorums.WithLocalServerOptions(opts...), + gorums.WithLocalDialOptions(InsecureDialOptions(t)), + ) if err != nil { t.Fatal(err) } diff --git a/handler.go b/handler.go index 40e81205b..f1e67ff72 100644 --- a/handler.go +++ b/handler.go @@ -72,51 +72,37 @@ func (ctx *ServerCtx) SendMessage(out *Message) { } } -// Config returns a [Configuration] of all connected known peer servers, including this node. -// An empty (non-nil) Configuration is returned if no known peers are connected. -// The returned slice is replaced atomically on each connect/disconnect; -// thus, retaining a reference to an old configuration is safe. -func (ctx *ServerCtx) Config() Configuration { +// PeerConfig returns the [Configuration] of the peers the server was configured +// with via [WithPeers], or nil if it was not used. It is the full peer set, not +// the currently reachable subset, so quorum sizes derived from it inside a +// handler do not shift as peers connect and disconnect. Use +// [ServerCtx.ConnectedPeers] to observe reachability. +func (ctx *ServerCtx) PeerConfig() Configuration { if ctx.srv == nil { return nil } - return ctx.srv.Config() + return ctx.srv.PeerConfig() } -// ClientConfig returns a [Configuration] of all connected clients capable of -// receiving reverse-direction calls from the server. -// An empty (non-nil) Configuration is returned if no client peers are connected. -// The returned slice is replaced atomically on each connect/disconnect; -// thus, retaining a reference to an old configuration is safe. -func (ctx *ServerCtx) ClientConfig() Configuration { +// ConnectedPeers returns the currently reachable subset of +// [ServerCtx.PeerConfig]; see [Server.ConnectedPeers]. +func (ctx *ServerCtx) ConnectedPeers() Configuration { if ctx.srv == nil { return nil } - return ctx.srv.ClientConfig() + return ctx.srv.ConnectedPeers() } -// ConfigContext returns a [ConfigContext] encapsulating the [Configuration] of -// all connected known peer servers, including this node. -func (ctx *ServerCtx) ConfigContext() *ConfigContext { - if ctx.srv == nil { - return nil - } - if cfg := ctx.srv.Config(); cfg != nil { - return cfg.Context(ctx) - } - return nil -} - -// ClientConfigContext returns a [ConfigContext] encapsulating the [Configuration] of -// all connected clients capable of receiving reverse-direction calls from the server. -func (ctx *ServerCtx) ClientConfigContext() *ConfigContext { +// ConnectedClients returns a [Configuration] of all connected clients capable of +// receiving reverse-direction calls from the server. +// An empty (non-nil) Configuration is returned if no client peers are connected. +// The returned slice is replaced atomically on each connect/disconnect; +// thus, retaining a reference to an old configuration is safe. +func (ctx *ServerCtx) ConnectedClients() Configuration { if ctx.srv == nil { return nil } - if cfg := ctx.srv.ClientConfig(); len(cfg) > 0 { - return cfg.Context(ctx) - } - return nil + return ctx.srv.ConnectedClients() } // NewResponseMessage creates a new response envelope based on the provided proto diff --git a/inbound_manager.go b/inbound_manager.go index 34a36fcba..84315356d 100644 --- a/inbound_manager.go +++ b/inbound_manager.go @@ -63,31 +63,34 @@ func metadataWithNodeID(id uint32) metadata.MD { // Clients that specify node ID 0 in their metadata are assumed to be capable // of receiving reverse-direction calls from the server. These clients are // accepted with auto-generated IDs and included in the ClientConfig. -// Client nodes are removed from the nodes map when they disconnect, while -// known peer nodes persist in the map to allow for reconnection. +// Client nodes are removed from clientNodes when they disconnect, while +// known peer nodes persist in knownNodes to allow for reconnection. // // inboundManager is safe for concurrent use. type inboundManager struct { mu sync.RWMutex myID uint32 // this server's own NodeID; always present in inboundCfg - nodes map[uint32]*Node // pre-created for known peers; client peers added on connect - config Configuration // auto-updated slice of known peer servers, sorted by ID + knownNodes map[uint32]*Node // pre-created configured peers, including self when configured + clientNodes map[uint32]*Node // dynamically assigned peer-capable clients + peerConfig Configuration // the server's peer Configuration; set once by setPeerConfig + config Configuration // auto-updated connectivity-filtered subset of peerConfig, sorted by ID + inboundCfg Configuration // auto-updated slice of known peers with an inbound stream, sorted by ID clientConfig Configuration // auto-updated slice of client peers, sorted by ID nextMsgID atomic.Uint64 // counter for server-initiated message IDs sendBufferSize uint // send buffer size for inbound channels handler stream.RequestHandler // handler for dispatching incoming requests on all inbound nodes onConfigChange func(Configuration) // optional; called after each known-peer config change - nextClientID uint32 // next ID to assign to a client peer + nextClientID uint64 // next candidate ID for a client peer; uint64 represents exhaustion configCh chan struct{} // closed and replaced on each config/clientConfig change; protected by mu stopCh chan struct{} // closed on shutdown to unblock waiters; never replaced stopOnce sync.Once // ensures stopCh is closed exactly once } // clientIDStart is the starting ID for dynamically assigned client peers. -// Chosen to be high enough to avoid collisions with typical known-peer IDs. +// Chosen to keep dynamically assigned IDs away from typical known-peer IDs. // The available ID space is [clientIDStart, math.MaxUint32], giving approximately -// 4.3 billion unique IDs before exhaustion. acceptClient rejects new peers if the -// counter reaches math.MaxUint32 to prevent silent wraparound. +// 4.3 billion candidate IDs before exhaustion. Configured peers may use IDs in +// this range; the allocator skips every occupied known-peer or client ID. const clientIDStart = 1 << 20 // newInboundManager creates an inboundManager for this server whose NodeID is myID. @@ -101,7 +104,8 @@ const clientIDStart = 1 << 20 func newInboundManager(myID uint32, opt NodeListOption, sendBuffer uint, onConfigChange func(Configuration), handler stream.RequestHandler) *inboundManager { im := &inboundManager{ myID: myID, - nodes: make(map[uint32]*Node), + knownNodes: make(map[uint32]*Node), + clientNodes: make(map[uint32]*Node), sendBufferSize: sendBuffer, handler: handler, onConfigChange: onConfigChange, @@ -125,16 +129,15 @@ func newInboundManager(myID uint32, opt NodeListOption, sendBuffer uint, onConfi func (im *inboundManager) Nodes() []*Node { im.mu.RLock() defer im.mu.RUnlock() - return slices.SortedFunc(maps.Values(im.nodes), func(a, b *Node) int { + return slices.SortedFunc(maps.Values(im.knownNodes), func(a, b *Node) int { return cmp.Compare(a.ID(), b.ID()) }) } -// Config returns a [Configuration] of all connected known peer servers, including this node. -// An empty (non-nil) Configuration is returned if no known peers are connected. -// The returned slice is replaced atomically on each connect/disconnect; -// thus, retaining a reference to an old configuration is safe. -func (im *inboundManager) Config() Configuration { +// ConnectedPeers returns the current connected-peer [Configuration]; see +// [Server.ConnectedPeers]. Before setPeerConfig installs a peer configuration, +// it falls back to the inbound view. +func (im *inboundManager) ConnectedPeers() Configuration { if im == nil { return nil } @@ -143,12 +146,42 @@ func (im *inboundManager) Config() Configuration { return im.config } -// ClientConfig returns a [Configuration] of all connected clients capable of +// setPeerConfig installs the server's peer [Configuration], from which the +// connected-peer view is derived. It is called once by [NewServer] after the +// peer configuration is built; stream-state changes observed before that are +// picked up by the rebuild here. +func (im *inboundManager) setPeerConfig(cfg Configuration) { + im.mu.Lock() + defer im.mu.Unlock() + im.peerConfig = cfg + im.rebuildConfig() +} + +// peerStreamChanged records that a dialed peer's outbound stream came up or +// went down and rebuilds the connected-peer view. It is registered as the +// stream-state callback for the server's outbound peer nodes; the new state +// is read directly from the nodes during the rebuild. +func (im *inboundManager) peerStreamChanged(uint32, bool) { + im.mu.Lock() + defer im.mu.Unlock() + im.rebuildConfig() +} + +// inboundPeers returns the known peers with an inbound stream open to this +// server, plus the local node. Test-only: production code observes +// connectivity through ConnectedPeers. +func (im *inboundManager) inboundPeers() Configuration { + im.mu.RLock() + defer im.mu.RUnlock() + return im.inboundCfg +} + +// ConnectedClients returns a [Configuration] of all connected clients capable of // receiving reverse-direction calls from the server. // An empty (non-nil) Configuration is returned if no client peers are connected. // The returned slice is replaced atomically on each connect/disconnect; // thus, retaining a reference to an old configuration is safe. -func (im *inboundManager) ClientConfig() Configuration { +func (im *inboundManager) ConnectedClients() Configuration { if im == nil { return nil } @@ -185,7 +218,7 @@ func (im *inboundManager) newNode(id uint32, addr string) (*Node, error) { } else { node = newInboundNode(id, addr, im.getMsgID, im.handler) } - im.nodes[id] = node + im.knownNodes[id] = node return node, nil } @@ -197,7 +230,7 @@ func (im *inboundManager) isKnown(id uint32) bool { } im.mu.RLock() defer im.mu.RUnlock() - _, ok := im.nodes[id] + _, ok := im.knownNodes[id] return ok } @@ -243,14 +276,14 @@ func (im *inboundManager) AcceptPeer(streamCtx context.Context, inboundStream st func (im *inboundManager) registerPeer(streamCtx context.Context, inboundStream stream.BidiStream, id uint32) (stream.PeerNode, func(), error) { im.mu.Lock() defer im.mu.Unlock() - node := im.nodes[id] + node := im.knownNodes[id] detach := node.attachStream(streamCtx, inboundStream, im.sendBufferSize) im.rebuildConfig() return node, func() { im.mu.Lock() defer im.mu.Unlock() - _, ok := im.nodes[id] + _, ok := im.knownNodes[id] if !ok { return } @@ -261,59 +294,91 @@ func (im *inboundManager) registerPeer(streamCtx context.Context, inboundStream } // acceptClient creates a new node with an auto-assigned ID for an unknown -// connecting client. The node is added to the nodes map and the configuration +// connecting client. The node is added to clientNodes and the configuration // is rebuilt. The returned cleanup function removes the client node entirely // when the stream ends (unlike known peers which persist for reconnection). func (im *inboundManager) acceptClient(streamCtx context.Context, inboundStream stream.BidiStream) (stream.PeerNode, func(), error) { im.mu.Lock() defer im.mu.Unlock() - if im.nextClientID == ^uint32(0) { - return nil, func() {}, fmt.Errorf("gorums: dynamic client ID space exhausted") + id, err := im.nextAvailableClientID() + if err != nil { + return nil, func() {}, err } - id := im.nextClientID - im.nextClientID++ node := newInboundNode(id, "client", im.getMsgID, im.handler) detach := node.attachStream(streamCtx, inboundStream, im.sendBufferSize) - im.nodes[id] = node + im.clientNodes[id] = node im.rebuildConfig() return node, func() { im.mu.Lock() defer im.mu.Unlock() - _, ok := im.nodes[id] + _, ok := im.clientNodes[id] if !ok { return } if detach() { - delete(im.nodes, id) + delete(im.clientNodes, id) im.rebuildConfig() } }, nil } -// rebuildConfig rebuilds inbound and client configurations from the current nodes map. -// A node is included in the known Config if it has an active channel (peer connected) -// or if it is myID. A node is included in ClientConfig if it is a connected client peer. +// nextAvailableClientID returns the next unoccupied dynamic client ID. +// The caller must hold im.mu. +func (im *inboundManager) nextAvailableClientID() (uint32, error) { + const maxNodeID = uint64(1<<32 - 1) + for im.nextClientID <= maxNodeID { + id := uint32(im.nextClientID) + im.nextClientID++ + if _, exists := im.knownNodes[id]; exists { + continue + } + if _, exists := im.clientNodes[id]; exists { + continue + } + return id, nil + } + return 0, fmt.Errorf("gorums: dynamic client ID space exhausted") +} + +// rebuildConfig rebuilds the inbound, client, and connected-peer +// configurations from their sources. A known peer is in the inbound +// configuration if it has an active channel (the peer opened a stream to this +// server) or if it is myID; a client is in the client configuration while its +// stream lives. The connected-peer configuration is the subset of the installed +// peer configuration whose nodes can currently carry calls; before a peer +// configuration is installed it falls back to the inbound view. // Callers must hold the lock. func (im *inboundManager) rebuildConfig() { - cfg := make(Configuration, 0, len(im.nodes)) - clientCfg := make(Configuration, 0) - for id, node := range im.nodes { - if id >= clientIDStart { - if node.channel.Load() != nil { - clientCfg = append(clientCfg, node) - } - } else { - if id == im.myID || node.channel.Load() != nil { + inboundCfg := make(Configuration, 0, len(im.knownNodes)) + for id, node := range im.knownNodes { + if id == im.myID || node.channel.Load() != nil { + inboundCfg = append(inboundCfg, node) + } + } + clientCfg := make(Configuration, 0, len(im.clientNodes)) + for _, node := range im.clientNodes { + if node.channel.Load() != nil { + clientCfg = append(clientCfg, node) + } + } + slices.SortFunc(inboundCfg, ID) + slices.SortFunc(clientCfg, ID) + im.inboundCfg = inboundCfg + im.clientConfig = clientCfg + + cfg := inboundCfg + if im.peerConfig != nil { + cfg = make(Configuration, 0, len(im.peerConfig)) + for _, node := range im.peerConfig { + if node.ID() == im.myID || node.isUp() { cfg = append(cfg, node) } } + slices.SortFunc(cfg, ID) } - slices.SortFunc(cfg, ID) - slices.SortFunc(clientCfg, ID) cfgChanged := !slices.Equal(im.config, cfg) im.config = cfg - im.clientConfig = clientCfg if cfgChanged && im.onConfigChange != nil { im.onConfigChange(cfg) } @@ -352,21 +417,29 @@ func (im *inboundManager) waitForConfig(ctx context.Context, cond func() bool) e } } -// waitForKnownConfig blocks until cond returns true for the current known-peer +// WaitForPeers blocks until cond returns true for the current connected-peer // [Configuration], or until ctx is cancelled or the server is stopped. -// The cond function receives the current known-peer configuration and must not -// acquire any additional locks. -func (im *inboundManager) waitForKnownConfig(ctx context.Context, cond func(Configuration) bool) error { +// The cond function receives the current connected-peer configuration and must +// not acquire any additional locks. +func (im *inboundManager) WaitForPeers(ctx context.Context, cond func(Configuration) bool) error { return im.waitForConfig(ctx, func() bool { return cond(im.config) }) } -// waitForClientConfig blocks until cond returns true for the current client-peer +// waitForInbound blocks until cond returns true for the current inbound view. +// Test-only counterpart of WaitForPeers. +func (im *inboundManager) waitForInbound(ctx context.Context, cond func(Configuration) bool) error { + return im.waitForConfig(ctx, func() bool { + return cond(im.inboundCfg) + }) +} + +// WaitForClients blocks until cond returns true for the current client-peer // [Configuration], or until ctx is cancelled or the server is stopped. // The cond function receives the current client-peer configuration and must not // acquire any additional locks. -func (im *inboundManager) waitForClientConfig(ctx context.Context, cond func(Configuration) bool) error { +func (im *inboundManager) WaitForClients(ctx context.Context, cond func(Configuration) bool) error { return im.waitForConfig(ctx, func() bool { return cond(im.clientConfig) }) @@ -396,11 +469,17 @@ func (p *nilPeerNode) RouteInbound(ctx context.Context, msg *stream.Message, rel } } -// Enqueue sends the message directly on the inbound stream. On the first send -// error the failure is latched and subsequent calls become no-ops, preventing -// wasted sends while gRPC propagates the stream-context cancellation that -// causes NodeStream to exit. -func (p *nilPeerNode) Enqueue(req stream.Request) { +// TrySend writes the message directly to the inbound stream. +// +// Unlike [Node.TrySend], this can still block: a plain client has no +// gorums-owned send queue, only the raw gRPC stream, whose Send blocks under +// HTTP/2 flow control with no non-blocking alternative. That is acceptable +// here because a stuck Send only stalls this one client's own NodeStream +// goroutine, not a lock shared with other connections. +// +// On the first send error the failure is latched and later calls become +// no-ops, avoiding wasted sends while the stream shuts down. +func (p *nilPeerNode) TrySend(req stream.Request) { if p.failed.Load() { return } diff --git a/inbound_manager_test.go b/inbound_manager_test.go index 936d4c208..3dad62be3 100644 --- a/inbound_manager_test.go +++ b/inbound_manager_test.go @@ -206,7 +206,7 @@ func TestNewInboundManager(t *testing.T) { t.Errorf("Node %d ID = %d; want %d", i, node.ID(), tc.wantIDs[i]) } } - if got := im.Config().NodeIDs(); !slices.Equal(got, tc.wantCfgIDs) { + if got := im.inboundPeers().NodeIDs(); !slices.Equal(got, tc.wantCfgIDs) { t.Errorf("Config().NodeIDs() = %v; want %v", got, tc.wantCfgIDs) } }) @@ -327,7 +327,7 @@ func TestAcceptPeerUpdatesConfig(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { im := newTestInboundManager(t, 1) - checkIDs(t, im.Config(), []uint32{1}, "initial") + checkIDs(t, im.inboundPeers(), []uint32{1}, "initial") cleanups := make(map[uint32]func()) for i, s := range tc.steps { @@ -344,7 +344,7 @@ func TestAcceptPeerUpdatesConfig(t *testing.T) { default: t.Fatalf("unknown op %q in step %d", s.op, i) } - checkIDs(t, im.Config(), s.wantIDs, fmt.Sprintf("step %d (%s id=%d)", i, s.op, s.id)) + checkIDs(t, im.inboundPeers(), s.wantIDs, fmt.Sprintf("step %d (%s id=%d)", i, s.op, s.id)) } }) } @@ -409,15 +409,15 @@ func TestAcceptPeerReplacesExistingStream(t *testing.T) { first := newMockBidiStream() t.Cleanup(first.close) im.AcceptPeer(inboundCtx(t.Context(), 3), first) - checkIDs(t, im.Config(), []uint32{1, 3}, "after first connect") + checkIDs(t, im.inboundPeers(), []uint32{1, 3}, "after first connect") // Peer 3 reconnects — second AcceptPeer must replace the first channel. second := newMockBidiStream() t.Cleanup(second.close) im.AcceptPeer(inboundCtx(t.Context(), 3), second) - checkIDs(t, im.Config(), []uint32{1, 3}, "after replacement") - node := im.nodes[3] + checkIDs(t, im.inboundPeers(), []uint32{1, 3}, "after replacement") + node := im.knownNodes[3] if ch := node.channel.Load(); ch == nil { t.Fatal("channel should not be nil after replacement") } @@ -435,7 +435,7 @@ func TestAcceptPeerStaleCleanupDoesNotDetachReplacement(t *testing.T) { if err != nil { t.Fatalf("AcceptPeer(first) error: %v", err) } - checkIDs(t, im.Config(), []uint32{1, 2}, "after first connect") + checkIDs(t, im.inboundPeers(), []uint32{1, 2}, "after first connect") second := newMockBidiStream() t.Cleanup(second.close) @@ -443,19 +443,19 @@ func TestAcceptPeerStaleCleanupDoesNotDetachReplacement(t *testing.T) { if err != nil { t.Fatalf("AcceptPeer(second) error: %v", err) } - checkIDs(t, im.Config(), []uint32{1, 2}, "after replacement") + checkIDs(t, im.inboundPeers(), []uint32{1, 2}, "after replacement") // Stale cleanup from the first connection must not detach the replacement. cleanupFirst() - checkIDs(t, im.Config(), []uint32{1, 2}, "after stale cleanup") - if im.nodes[2].channel.Load() == nil { + checkIDs(t, im.inboundPeers(), []uint32{1, 2}, "after stale cleanup") + if im.knownNodes[2].channel.Load() == nil { t.Fatal("stale cleanup detached the replacement channel") } // Current cleanup should detach the active channel. cleanupSecond() - checkIDs(t, im.Config(), []uint32{1}, "after current cleanup") - if im.nodes[2].channel.Load() != nil { + checkIDs(t, im.inboundPeers(), []uint32{1}, "after current cleanup") + if im.knownNodes[2].channel.Load() != nil { t.Fatal("current cleanup should detach the active channel") } } @@ -588,24 +588,24 @@ func TestOnConfigChangeCallbackIdempotentCleanup(t *testing.T) { } } -// mustWaitForConfig blocks until cond returns true for srv's known-peer +// mustWaitForInbound blocks until cond returns true for srv's inbound peer // Configuration, or fails the test after a 2-second timeout. -func mustWaitForConfig(t *testing.T, srv *Server, cond func(Configuration) bool) { +func mustWaitForInbound(t *testing.T, srv *Server, cond func(Configuration) bool) { t.Helper() ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) defer cancel() - if err := srv.waitForKnownConfig(ctx, cond); err != nil { + if err := srv.waitForInbound(ctx, cond); err != nil { t.Fatalf("waitForKnownConfig: %v", err) } } -// mustWaitForClientConfig blocks until cond returns true for srv's client-peer +// mustWaitForClients blocks until cond returns true for srv's client-peer // Configuration, or fails the test after a 2-second timeout. -func mustWaitForClientConfig(t *testing.T, srv *Server, cond func(Configuration) bool) { +func mustWaitForClients(t *testing.T, srv *Server, cond func(Configuration) bool) { t.Helper() ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) defer cancel() - if err := srv.waitForClientConfig(ctx, cond); err != nil { + if err := srv.WaitForClients(ctx, cond); err != nil { t.Fatalf("waitForClientConfig: %v", err) } } @@ -616,7 +616,7 @@ func testPeerServer(t *testing.T) (*Server, []string) { t.Helper() var srv *Server addrs := testStartServers(t, 1, func(_ int) ServerIface { - srv = NewServer(WithConfig(1, peerNodes())) + srv = NewServer(WithPeers(1, peerNodes(), testDialOptions(t))) return srv }) return srv, addrs @@ -658,12 +658,12 @@ func connectAsPeer(t *testing.T, peerID uint32, addrs []string) Configuration { func TestKnownPeerConnects(t *testing.T) { srv, addrs := testPeerServer(t) - checkIDs(t, srv.Config(), []uint32{1}, "before connect") + checkIDs(t, srv.inboundPeers(), []uint32{1}, "before connect") connectAsPeer(t, 2, addrs) - mustWaitForConfig(t, srv, equalNodeIDs([]uint32{1, 2})) - checkIDs(t, srv.Config(), []uint32{1, 2}, "after connect") + mustWaitForInbound(t, srv, equalNodeIDs([]uint32{1, 2})) + checkIDs(t, srv.inboundPeers(), []uint32{1, 2}, "after connect") } // TestKnownPeerDisconnects verifies that when a peer closes its @@ -673,15 +673,15 @@ func TestKnownPeerDisconnects(t *testing.T) { srv, addrs := testPeerServer(t) cfg := connectAsPeer(t, 2, addrs) - mustWaitForConfig(t, srv, equalNodeIDs([]uint32{1, 2})) + 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) } - mustWaitForConfig(t, srv, equalNodeIDs([]uint32{1})) - checkIDs(t, srv.Config(), []uint32{1}, "after disconnect") + mustWaitForInbound(t, srv, equalNodeIDs([]uint32{1})) + checkIDs(t, srv.inboundPeers(), []uint32{1}, "after disconnect") } // TestUnknownPeerIgnored verifies that a client sending an @@ -700,7 +700,7 @@ func TestUnknownPeerIgnored(t *testing.T) { // Give the server time to process both connections. time.Sleep(50 * time.Millisecond) - checkIDs(t, srv.Config(), []uint32{1}, "external and unknown peers must not appear") + checkIDs(t, srv.inboundPeers(), []uint32{1}, "external and unknown peers must not appear") } // TestKnownPeerServerCallsClient verifies the full symmetric communication path: @@ -717,17 +717,17 @@ func TestKnownPeerServerCallsClient(t *testing.T) { return NewResponseMessage(in, pb.String("echo: "+req.GetValue())), nil }) peerMD := metadata.Pairs(gorumsNodeIDKey, "2") - cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithMetadata(peerMD), WithServer(clientSrv)) + cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithMetadata(peerMD), WithBackChannel(clientSrv)) if err != nil { t.Fatalf("NewConfig() error: %v", err) } t.Cleanup(testCloser(t, cfg)) // Wait for the peer to appear in the inbound config. - mustWaitForConfig(t, srv, equalNodeIDs([]uint32{1, 2})) + mustWaitForInbound(t, srv, equalNodeIDs([]uint32{1, 2})) // Server sends a request to the client via the inbound node. - inboundCfg := srv.Config() + inboundCfg := srv.inboundPeers() var peerNode *Node for _, n := range inboundCfg.Nodes() { if n.ID() == 2 { @@ -795,7 +795,7 @@ func testClientServer(t *testing.T) (*Server, []string) { // ClientConfig and may dispatch server-initiated calls to it. func connectAsPeerClient(t *testing.T, addrs []string) Configuration { t.Helper() - cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithServer(NewServer())) + cfg, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithBackChannel(NewServer())) if err != nil { t.Fatalf("NewConfig() error: %v", err) } @@ -809,13 +809,13 @@ func TestClientConfigConnects(t *testing.T) { srv, addrs := testClientServer(t) // Initially no peers (no self-node since myID == 0) - checkIDs(t, srv.ClientConfig(), []uint32{}, "before connect") + checkIDs(t, srv.ConnectedClients(), []uint32{}, "before connect") connectAsPeerClient(t, addrs) // Client peer should appear with auto-assigned ID >= clientIDStart. - mustWaitForClientConfig(t, srv, func(cfg Configuration) bool { return len(cfg) > 0 }) - cfg := srv.ClientConfig() + mustWaitForClients(t, srv, func(cfg Configuration) bool { return len(cfg) > 0 }) + cfg := srv.ConnectedClients() if len(cfg) != 1 { t.Fatalf("ClientConfig has %d nodes; want 1", len(cfg)) } @@ -832,9 +832,9 @@ func TestClientConfigDisconnects(t *testing.T) { cfg := connectAsPeerClient(t, addrs) // Wait for the client peer to appear. - mustWaitForClientConfig(t, srv, func(cfg Configuration) bool { return len(cfg) > 0 }) - if len(srv.ClientConfig()) != 1 { - t.Fatalf("ClientConfig has %d nodes; want 1", len(srv.ClientConfig())) + mustWaitForClients(t, srv, func(cfg Configuration) bool { return len(cfg) > 0 }) + if len(srv.ConnectedClients()) != 1 { + t.Fatalf("ClientConfig has %d nodes; want 1", len(srv.ConnectedClients())) } // Disconnect the client peer. @@ -843,35 +843,35 @@ func TestClientConfigDisconnects(t *testing.T) { } // Wait for config to become empty. - mustWaitForClientConfig(t, srv, func(cfg Configuration) bool { return len(cfg) == 0 }) - checkIDs(t, srv.ClientConfig(), []uint32{}, "after disconnect") + mustWaitForClients(t, srv, func(cfg Configuration) bool { return len(cfg) == 0 }) + checkIDs(t, srv.ConnectedClients(), []uint32{}, "after disconnect") } -// TestClientConfigMixedMode verifies that a server with both WithConfig and +// TestClientConfigMixedMode verifies that a server with both WithPeers and // WithClientConfig accepts known peers by ID and unknown clients dynamically. func TestClientConfigMixedMode(t *testing.T) { srv, addrs := testPeerServer(t) // Self-node (ID 1) is present initially. - checkIDs(t, srv.Config(), []uint32{1}, "before connect") + checkIDs(t, srv.inboundPeers(), []uint32{1}, "before connect") // Connect known peer (ID 2). connectAsPeer(t, 2, addrs) - mustWaitForConfig(t, srv, equalNodeIDs([]uint32{1, 2})) + mustWaitForInbound(t, srv, equalNodeIDs([]uint32{1, 2})) // Connect peer-capable anonymous client (dynamic peer). connectAsPeerClient(t, addrs) // Wait for 1 dynamic node. - mustWaitForClientConfig(t, srv, func(cfg Configuration) bool { return len(cfg) == 1 }) - dynCfg := srv.ClientConfig() + mustWaitForClients(t, srv, func(cfg Configuration) bool { return len(cfg) == 1 }) + dynCfg := srv.ConnectedClients() if len(dynCfg) != 1 { t.Fatalf("ClientConfig has %d nodes; want 1", len(dynCfg)) } if dynCfg[0].ID() < clientIDStart { t.Errorf("Client peer ID = %d; want >= %d", dynCfg[0].ID(), clientIDStart) } - cfg := srv.Config() + cfg := srv.inboundPeers() if len(cfg) != 2 { t.Fatalf("Config has %d nodes; want 2", len(cfg)) } @@ -882,13 +882,13 @@ func TestClientConfigMixedMode(t *testing.T) { } // TestClientConfigServerCallsClient verifies that a server dispatches a reverse-direction -// multicast to a connected client via [ServerCtx.ClientConfigContext]. +// multicast to a connected client via [ServerCtx.ConnectedClients]. func TestClientConfigServerCallsClient(t *testing.T) { // Register the server handler before starting so it is present before clients arrive. srv := NewServer() srv.RegisterHandler(mock.TestMethod, func(ctx ServerCtx, _ *Message) (*Message, error) { - if cc := ctx.ClientConfigContext(); cc != nil { - _ = Multicast(cc, pb.String("ping"), mock.Stream) + if clients := ctx.ConnectedClients(); len(clients) > 0 { + _ = Multicast(clients.Context(ctx), pb.String("ping"), mock.Stream) } return nil, nil // one-way }) @@ -903,14 +903,14 @@ func TestClientConfigServerCallsClient(t *testing.T) { wg.Done() return nil, nil }) - clientConfig, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithServer(clientSrv)) + clientConfig, err := NewConfig(WithNodeList(addrs), testDialOptions(t), WithBackChannel(clientSrv)) if err != nil { t.Fatalf("NewConfig() error: %v", err) } t.Cleanup(testCloser(t, clientConfig)) // Wait for the client to appear in the server's ClientConfig. - mustWaitForClientConfig(t, srv, func(cfg Configuration) bool { return len(cfg) > 0 }) + mustWaitForClients(t, srv, func(cfg Configuration) bool { return len(cfg) > 0 }) // Trigger: client multicasts TestMethod to the server; server fans it back via ClientConfig. ctx := testTimeoutContext(t, 2*time.Second) diff --git a/internal/stream/channel.go b/internal/stream/channel.go index 8c4884f44..e0f58a816 100644 --- a/internal/stream/channel.go +++ b/internal/stream/channel.go @@ -4,6 +4,7 @@ import ( "cmp" "context" "sync" + "sync/atomic" "time" "google.golang.org/grpc" @@ -13,8 +14,17 @@ import ( ) var ( + // ErrNodeClosed is returned for requests enqueued after the node closed. ErrNodeClosed = status.Error(codes.Unavailable, "node closed") + // ErrStreamDown is returned for requests that cannot be delivered or + // retried because the node's stream is not available. ErrStreamDown = status.Error(codes.Unavailable, "stream is down") + // ErrSendQueueFull is returned for two-way requests enqueued while the + // node's send queue is at capacity. A full queue means the peer is not + // draining sends (stopped reading, exhausted flow control); failing fast + // lets quorum logic count the peer as failed instead of stalling the + // caller behind it. One-way requests block instead (see Enqueue). + ErrSendQueueFull = status.Error(codes.Unavailable, "send queue full") ) // BidiStream abstracts both client-side and server-side bidirectional streams. @@ -43,7 +53,10 @@ func (r Request) wantServerResponse() bool { // wantSendConfirmation returns true if the request needs send confirmation // delivered directly on its ResponseChan, bypassing the router. It returns -// true for one-way calls (Unicast, Multicast) that are not fire-and-forget. +// true for one-way calls (Unicast, Multicast), whose callers await the +// confirmation to learn whether the send succeeded. The nil check guards +// against delivering to a nil channel, which blocks until the request's +// context expires. func (r Request) wantSendConfirmation() bool { return r.Oneway && r.ResponseChan != nil } @@ -65,8 +78,10 @@ func (r Request) deliver(resp response) bool { } } -// replyError sends err to the request's response channel if one is set. -func (r Request) replyError(nodeID uint32, err error) { +// ReplyError sends err to the request's response channel if one is set. +// It is exported so callers outside this package can fail a request that +// never reaches a channel (e.g., a node with no attached channel). +func (r Request) ReplyError(nodeID uint32, err error) { if r.ResponseChan != nil { r.deliver(response{NodeID: nodeID, Err: err}) } @@ -95,10 +110,37 @@ type Channel struct { streamCancel context.CancelFunc streamReady chan struct{} // signals receiver when stream becomes available + // eagerReconnect makes the receiver re-establish a lost stream proactively + // instead of waiting for the next local send; see [NewOutboundChannel]. + eagerReconnect bool + + // streamUp mirrors whether the outbound stream is currently established, + // so [Channel.StreamUp] can answer without taking streamMut. Maintained by + // setStreamUp on every stream transition; always false for inbound and + // local channels, which report their state structurally instead. + streamUp atomic.Bool + + // onStreamChange, if non-nil, is invoked on every outbound stream + // transition with the new state; see [NewOutboundChannel]. It is called + // while internal locks are held, so it must not call back into the + // Channel; use it only to signal or record the state elsewhere. + onStreamChange func(up bool) + + // sendGuard serializes each request's post-Send bookkeeping in the sender + // against that request's cancel watcher, so the watcher can distinguish a + // Send still in flight (which it must unblock by clearing the stream) from + // one that has already returned (the stream is healthy and must be left + // alone); see the sender loop. + sendGuard sync.Mutex + // Router handles response routing for pending calls. It is owned by the // Node and injected into the Channel, so it survives channel replacement. router *MessageRouter + pendingOwner *pendingOwner closeOnceFunc func() error + + // droppedReplies counts replies silently dropped by trySend: see DroppedReplies. + droppedReplies atomic.Int64 } // NewOutboundChannel creates a new channel for the given node and starts @@ -108,8 +150,21 @@ type Channel struct { // have not yet been established. This is to prevent deadlock when invoking // a call type. The sender blocks on the sendQ and the receiver waits for // the stream to become available. -func NewOutboundChannel(parentCtx context.Context, id uint32, sendBufferSize uint, conn *grpc.ClientConn, router *MessageRouter) *Channel { - return newChannel(parentCtx, id, sendBufferSize, conn, nil, router) +// +// When eagerReconnect is set, the receiver re-establishes a lost stream +// proactively with capped exponential backoff instead of waiting for the next +// local send. Use this whenever a remote peer depends on this dialed stream +// staying registered on its inbound side. Any symmetric peer (a server calling +// its peers via WithPeers) drops out of the remote's connected +// configuration when the stream it dialed goes idle and dies. A stream lost +// while this side has nothing to send would otherwise leave the peer stalled +// until the next local send. +// +// onStreamChange, if non-nil, is invoked with true when the stream is +// established and false when it is lost, on transitions only. It runs while +// internal locks are held and must not call back into the Channel. +func NewOutboundChannel(parentCtx context.Context, id uint32, sendBufferSize uint, conn *grpc.ClientConn, router *MessageRouter, eagerReconnect bool, onStreamChange func(up bool)) *Channel { + return newChannel(parentCtx, id, sendBufferSize, conn, nil, router, eagerReconnect, onStreamChange) } // NewInboundChannel creates a channel from an existing server-side stream. @@ -127,7 +182,7 @@ func NewOutboundChannel(parentCtx context.Context, id uint32, sendBufferSize uin // - Cannot reconnect (the client controls stream creation) // - Close only cancels context; it does not close the underlying connection func NewInboundChannel(parentCtx context.Context, id uint32, sendBufferSize uint, stream BidiStream, router *MessageRouter) *Channel { - return newChannel(parentCtx, id, sendBufferSize, nil, stream, router) + return newChannel(parentCtx, id, sendBufferSize, nil, stream, router, false, nil) } // newChannel is the shared constructor for outbound and inbound channels. @@ -135,21 +190,25 @@ func NewInboundChannel(parentCtx context.Context, id uint32, sendBufferSize uint // Pass a non-nil stream for inbound channels (stream is immediately ready; no reconnection). // The receiver goroutine is started only for outbound channels; inbound callers own // the stream's read side themselves (see NewInboundChannel for the full rationale). -func newChannel(parentCtx context.Context, id uint32, sendBufferSize uint, conn *grpc.ClientConn, stream BidiStream, router *MessageRouter) *Channel { +func newChannel(parentCtx context.Context, id uint32, sendBufferSize uint, conn *grpc.ClientConn, stream BidiStream, router *MessageRouter, eagerReconnect bool, onStreamChange func(up bool)) *Channel { connCtx, connCancel := context.WithCancel(parentCtx) c := &Channel{ - sendQ: make(chan Request, sendBufferSize), - id: id, - conn: conn, - stream: stream, - connCtx: connCtx, - connCancel: connCancel, - router: router, - streamReady: make(chan struct{}, 1), + sendQ: make(chan Request, sendBufferSize), + id: id, + conn: conn, + stream: stream, + connCtx: connCtx, + connCancel: connCancel, + router: router, + pendingOwner: new(pendingOwner), + streamReady: make(chan struct{}, 1), + eagerReconnect: eagerReconnect, + onStreamChange: onStreamChange, } c.closeOnceFunc = sync.OnceValue(func() error { // important to cancel first to stop goroutines connCancel() + c.setStreamUp(false) // unblocks any pending senders/receivers c.cancelPendingMsgs(ErrNodeClosed) if conn != nil { @@ -178,8 +237,9 @@ func newChannel(parentCtx context.Context, id uint32, sendBufferSize uint, conn // No goroutines are started; the channel's Close is a no-op. func NewLocalChannel(id uint32, router *MessageRouter) *Channel { c := &Channel{ - id: id, - router: router, + id: id, + router: router, + pendingOwner: new(pendingOwner), } c.closeOnceFunc = sync.OnceValue(func() error { return nil }) return c @@ -199,12 +259,9 @@ func (c *Channel) IsInbound() bool { return c.conn == nil && c.sendQ != nil } -// NewChannelWithState creates a new Channel with a specific state for testing. -// This function should only be used in tests. -func NewChannelWithState(lastErr error) *Channel { - return &Channel{ - lastError: lastErr, - } +// IsOutbound returns true if this channel was created as an outbound client connection. +func (c *Channel) IsOutbound() bool { + return c.conn != nil } // Close closes the channel and the underlying connection exactly once. @@ -212,20 +269,27 @@ func (c *Channel) Close() error { return c.closeOnceFunc() } -// ensureStream ensures there is an active NodeStream for the sender and -// receiver goroutines, and signals the receiver when the stream is ready. +// ensureStream ensures there is an active NodeStream, signals the receiver +// that the stream is ready, and returns the ensured stream. The caller must +// use the returned stream rather than re-reading it with getStream: a +// concurrent clearStream — the receiver observing a broken stream, or a +// cancel watcher — can clear the stream between the two steps, and a request +// that never obtains a stream is failed without ever being registered for +// retry. Sending on the returned stream after such a clear fails instead with +// a stream error, after registration, so the request is requeued. // gRPC automatically handles TCP connection state when creating the stream. // This method is safe for concurrent use. -func (c *Channel) ensureStream() error { +func (c *Channel) ensureStream() (BidiStream, error) { if c.IsInbound() { // Inbound channels cannot reconnect; just check if stream exists. - if c.getStream() == nil { - return ErrStreamDown + if stream := c.getStream(); stream != nil { + return stream, nil } - return nil + return nil, ErrStreamDown } - if err := c.ensureConnectedNodeStream(); err != nil { - return err + stream, err := c.ensureConnectedNodeStream() + if err != nil { + return nil, err } // signal receiver that stream is ready (non-blocking) select { @@ -233,23 +297,29 @@ func (c *Channel) ensureStream() error { default: // channel already has a signal pending, no need to add another } - return nil + return stream, nil } -// ensureConnectedNodeStream ensures there is an active and connected -// NodeStream, or creates a new stream if one doesn't already exist. +// ensureConnectedNodeStream returns the active NodeStream over a ready +// connection, creating a new stream if there is none. // This method is safe for concurrent use. -func (c *Channel) ensureConnectedNodeStream() (err error) { +func (c *Channel) ensureConnectedNodeStream() (BidiStream, error) { c.streamMut.Lock() defer c.streamMut.Unlock() // if we already have a ready connection and an active stream, do nothing - // (cannot reuse isConnected() here since it uses the streamMut lock) if c.conn.GetState() == connectivity.Ready && c.stream != nil { - return nil + return c.stream, nil + } + // Cancel any stream left behind by a previous attempt before replacing + // it, so it does not stay alive server-side as an orphan. + if c.streamCancel != nil { + c.streamCancel() } c.streamCtx, c.streamCancel = context.WithCancel(c.connCtx) + var err error c.stream, err = NewGorumsClient(c.conn).NodeStream(c.streamCtx) - return err + c.setStreamUp(c.stream != nil) + return c.stream, err } // getStream returns the current stream, or nil if no stream is available. @@ -277,25 +347,50 @@ func (c *Channel) clearStream(stale BidiStream) bool { c.streamCancel() } c.stream = nil + c.setStreamUp(false) return true } -// isConnected returns true if the channel has an active stream. -// For outbound channels, also requires the gRPC connection to be in Ready state. -// This method is safe for concurrent use. -func (c *Channel) isConnected() bool { - if c.IsInbound() { - return c.connCtx.Err() == nil && c.getStream() != nil +// setStreamUp records the outbound stream's availability and invokes the +// registered onStreamChange callback on transitions only. The compare-and-swap +// makes repeated same-state calls no-ops, so callers may invoke it +// unconditionally after each stream mutation. +func (c *Channel) setStreamUp(up bool) { + if c.streamUp.CompareAndSwap(!up, up) && c.onStreamChange != nil { + c.onStreamChange(up) + } +} + +// StreamUp reports whether the channel can currently carry requests, without +// taking locks: local channels always can, inbound channels can for as long +// as they exist (they are discarded when their stream ends), and outbound +// channels can while their stream is established. +func (c *Channel) StreamUp() bool { + if c.isLocal() || c.IsInbound() { + return true } - return c.conn.GetState() == connectivity.Ready && c.getStream() != nil + return c.streamUp.Load() } -// Enqueue adds the request to the send queue. +// Enqueue adds the request to the send queue, blocking the caller if the +// queue is full. // // If it is a local channel, the request is dispatched in-process via // the registered RequestHandler without touching the network. // If the node is closed, it responds with an error instead. // +// Two-way requests never wait here: a full queue means the peer is not +// draining sends, and failing fast with ErrSendQueueFull beats stalling every +// caller behind one slow peer (see [Channel.trySend]). One-way client calls +// (Unicast, Multicast) do wait: with no reply to await, backpressure on the +// caller is the only thing pacing the producer. Both wait points (here and at +// the sender's dequeue) honor the request's context, so a bounded or +// cancellable context still releases the caller; a context with no deadline +// can block indefinitely behind a peer that stopped draining. +// +// Enqueue must never be used for a reply sent from a receive/dispatch loop — +// use [Channel.trySend] instead. See [Channel.dispatchInbound]. +// // Requests cannot combine Oneway and Streaming; they are mutually exclusive: // - one-way calls (Unicast, Multicast) do not expect server responses. // - streaming (correctable) calls expect multiple server responses and @@ -315,56 +410,150 @@ func (c *Channel) Enqueue(req Request) { // other case is ready, so if connCtx.Done() is already closed it always // wins — unlike a plain single select, where Go randomly picks between a // ready Done channel and a buffered sendQ. - // The inner select handles the case where the node closes concurrently + // The inner selects handle the case where the node closes concurrently // while we are waiting for sendQ space; there a narrow race remains, but - // drainSendQ (deferred in sender) will drain and replyError any entry that + // drainSendQ (deferred in sender) will drain and ReplyError any entry that // slips through after sender exits. select { case <-c.connCtx.Done(): // the node's close() method was called: respond with error instead of enqueueing - req.replyError(c.id, ErrNodeClosed) + req.ReplyError(c.id, ErrNodeClosed) + return default: - select { - case <-c.connCtx.Done(): - // the node's close() method was called: respond with error instead of enqueueing - req.replyError(c.id, ErrNodeClosed) - case c.sendQ <- req: - // enqueued successfully + } + if req.wantServerResponse() { + // Two-way request: never wait for queue space. + c.trySend(req) + return + } + select { + case <-c.connCtx.Done(): + // the node's close() method was called: respond with error instead of enqueueing + req.ReplyError(c.id, ErrNodeClosed) + case <-req.Ctx.Done(): + // The request's own context was cancelled while waiting for queue space. + // Without this case a caller could block here indefinitely behind a peer + // that stopped reading; the sender applies the same check when it dequeues, + // so both wait points honor the request context. + req.ReplyError(c.id, req.Ctx.Err()) + case c.sendQ <- req: + // enqueued successfully + } +} + +// TrySend is [Channel.trySend] exported for callers outside this package — +// currently the server-side inbound reply path; see [PeerNode.TrySend]. Like +// trySend, it never blocks on network I/O; unlike trySend, a local channel's +// in-process dispatch can briefly block acquiring the dispatch lock. +func (c *Channel) TrySend(req Request) { + if c.isLocal() { + c.router.DispatchLocalRequest(c.id, req) + return + } + c.trySend(req) +} + +// trySend enqueues req without ever blocking the caller: if the node has +// closed it replies ErrNodeClosed, and if the send queue is full it replies +// ErrSendQueueFull instead of waiting for space. A request with no +// ResponseChan (a back-channel reply) is simply dropped when the queue is +// full, since there is no channel to deliver the error on; each such drop is +// counted (see [Channel.DroppedReplies]). +// +// Two callers rely on this never blocking: two-way requests (see [Channel.Enqueue] +// for why a full queue should fail fast rather than stall the caller), and +// replies sent from a receive/dispatch loop, which must keep reading inbound +// frames and would deadlock if a reply blocked instead — see +// [Channel.dispatchInbound] for the client-side case and [Server.NodeStream] +// for the server-side case. +func (c *Channel) trySend(req Request) { + // Deterministic already-closed check: see the equivalent select in Enqueue. + select { + case <-c.connCtx.Done(): + if req.ResponseChan == nil { + c.droppedReplies.Add(1) } + req.ReplyError(c.id, ErrNodeClosed) + return + default: } + select { + case c.sendQ <- req: + // enqueued successfully + default: + if req.ResponseChan == nil { + c.droppedReplies.Add(1) + } + req.ReplyError(c.id, ErrSendQueueFull) + } +} + +// DroppedReplies returns the number of replies this channel has silently +// dropped: requests with no ResponseChan (back-channel or inbound replies +// dispatched from a receive/dispatch loop) that [Channel.trySend] could not +// enqueue because the node had closed or the send queue was full. Two-way +// requests are never counted here, since their caller already observes the +// failure directly via ErrSendQueueFull or ErrNodeClosed. +func (c *Channel) DroppedReplies() int64 { + return c.droppedReplies.Load() } -// cancelPendingMsgs cancels all pending messages by sending an error response to each. -// This is called during node shutdown to notify all waiting calls. +// cancelPendingMsgs cancels this channel's pending messages by sending an +// error response to each. func (c *Channel) cancelPendingMsgs(err error) { - for _, req := range c.router.CancelPending() { - req.replyError(c.id, err) + for _, req := range c.router.cancelPending(c.pendingOwner) { + req.ReplyError(c.id, err) + } +} + +// cancelInflightSend is the sender's per-request cancel watcher: it clears +// the stream to unblock a Send that the request's canceled context would +// otherwise leave blocked forever (a Send stalled by flow control returns +// only when its stream dies), requeueing the pending requests stranded on the +// cleared stream. +// +// sendDone — set by the sender under sendGuard once Send returns — makes a +// watcher that runs late a no-op. The caller may cancel its context the +// moment it has the response, landing the cancellation between Send returning +// and the sender's stop call, and the watcher goroutine spawned by that +// cancellation may then run arbitrarily late; with nothing left to unblock, +// clearing would sever a healthy stream that later requests depend on. +// +// One narrow window remains: between Send returning and the sender acquiring +// sendGuard to set sendDone, a watcher can win the guard, observe sendDone +// still false, and clear a stream whose Send already completed. This is +// accepted rather than closed because it is self-healing and strands nothing: +// the requeued requests retry, and the stream is re-established on the next +// send (immediately when eager reconnect is set). The cost is a spurious +// reconnect, not lost or misrouted traffic. +func (c *Channel) cancelInflightSend(sendDone *bool, stream BidiStream) { + c.sendGuard.Lock() + defer c.sendGuard.Unlock() + if *sendDone { + return + } + if c.clearStream(stream) { + c.requeuePendingMsgs() } } // requeuePendingMsgs moves pending non-streaming requests back to sendQ for // retry on the next stream. Streaming requests (correctable calls) are cancelled // with ErrStreamDown because they cannot be safely retried. +// +// Only two-way requests are registered in the router, so every requeued entry +// takes Enqueue's non-blocking fail-fast path. Calling Enqueue directly from +// the sender goroutine (the sole sendQ reader) therefore cannot deadlock; +// entries that no longer fit are failed with ErrSendQueueFull rather than +// retried. If the node closed meanwhile, Enqueue replies ErrNodeClosed and +// drainSendQ (deferred in sender) drains any entries that slipped through. func (c *Channel) requeuePendingMsgs() { - requeue, cancel := c.router.RequeuePending() + requeue, cancel := c.router.requeuePending(c.pendingOwner) for _, req := range cancel { - req.replyError(c.id, ErrStreamDown) - } - // The requeue is performed in a separate goroutine because this method is called - // from sender(), which is the sole reader of sendQ. Calling Enqueue directly from - // the sender would deadlock: Enqueue writes to the unbuffered sendQ, but no one - // is reading it because the sender itself is blocked in the Enqueue call. - // The goroutine writes to sendQ while the sender returns to its read loop. - // - // If connCtx is cancelled before the goroutine finishes, each Enqueue call will - // take the connCtx.Done() branch and replyError with ErrNodeClosed, and - // drainSendQ (deferred in sender) will drain any items that made it to sendQ. - if len(requeue) > 0 { - go func() { - for _, req := range requeue { - c.Enqueue(req) - } - }() + req.ReplyError(c.id, ErrStreamDown) + } + for _, req := range requeue { + c.Enqueue(req) } } @@ -378,7 +567,7 @@ func (c *Channel) drainSendQ() { for { select { case req := <-c.sendQ: - req.replyError(c.id, ErrNodeClosed) + req.ReplyError(c.id, ErrNodeClosed) default: // sendQ is empty return @@ -390,10 +579,10 @@ func (c *Channel) drainSendQ() { // If the stream is down, it tries to re-establish it. // // Delivery contract: -// - Pre-registration exits (stream error, cancelled request context, nil stream): -// replyError + continue. The request never enters the router. +// - Pre-registration exits (stream ensure error, cancelled request context): +// ReplyError + continue. The request never enters the router. // - Send failure: requeuePendingMsgs handles registered two-way entries (requeue or cancel). -// One-way errors are delivered directly via replyError. +// One-way errors are delivered directly via ReplyError. // - Send success, one-way call: confirm send directly on ResponseChan. // - Send success, two-way call: the router entry stays alive for receiver() // to deliver the actual server response. @@ -401,7 +590,7 @@ func (c *Channel) sender() { defer c.drainSendQ() // eager connect; ignored if stream is down (will be retried on send) - _ = c.ensureStream() + _, _ = c.ensureStream() var req Request for { @@ -413,53 +602,56 @@ func (c *Channel) sender() { // take next request from sendQ } - if err := c.ensureStream(); err != nil { - req.replyError(c.id, err) + stream, err := c.ensureStream() + if err != nil { + // Failing to reach the peer is a fact about the node, not only + // about this request: record it for [Channel.LastErr] before + // reporting it to the caller. + c.recordHealth(err) + req.ReplyError(c.id, err) continue } if req.Ctx.Err() != nil { - req.replyError(c.id, req.Ctx.Err()) - continue - } - stream := c.getStream() - if stream == nil { - req.replyError(c.id, ErrStreamDown) + req.ReplyError(c.id, req.Ctx.Err()) continue } // One-way calls bypass the router and confirm directly after Send below. if req.wantServerResponse() { // Register only for two-way/streaming calls that expect server responses. - c.router.Register(req.Msg.GetMessageSeqNo(), req) + c.router.register(c.pendingOwner, req.Msg.GetMessageSeqNo(), req) } - // Watch for per-request cancellation while Send is in-flight. - // If req.Ctx is done before Send returns, clearStream unblocks - // the blocked Send by cancelling the stream context and the - // goroutine that wins the clear is also responsible for - // requeueing/canceling pending requests. - // - // stop() is advisory only: it may return false if the callback - // already started around the same time Send returned. + // Watch for per-request cancellation while Send is in flight: a Send + // blocked by flow control returns only when its stream dies, so the + // watcher unblocks it by clearing the stream. sendDone, set under + // sendGuard once Send returns, neutralizes a watcher that fires late: + // the caller may cancel its context the moment the response arrives — + // before this goroutine resumes to call stop — and the watcher + // goroutine spawned by that cancellation may then run arbitrarily + // late; see [Channel.cancelInflightSend]. + var sendDone bool stop := context.AfterFunc(req.Ctx, func() { - if c.clearStream(stream) { - c.requeuePendingMsgs() - } + c.cancelInflightSend(&sendDone, stream) }) - if err := stream.Send(req.Msg); err != nil { - stop() - c.setLastErr(err) + err = stream.Send(req.Msg) + c.sendGuard.Lock() + sendDone = true + c.sendGuard.Unlock() + stop() + // A completed send proves the channel usable; a failed one condemns it. + c.recordHealth(err) + if err != nil { c.clearStream(stream) c.requeuePendingMsgs() // handles registered two-way entries // One-way calls are not registered in the router to receive server responses, // so requeuePendingMsgs won't handle them. Deliver error directly to caller. if !req.wantServerResponse() { // prefer context error when cancellation caused the failure. - req.replyError(c.id, cmp.Or(req.Ctx.Err(), err)) + req.ReplyError(c.id, cmp.Or(req.Ctx.Err(), err)) } continue } - stop() // For one-way calls, confirm successful send directly (no router round-trip). if req.wantSendConfirmation() { @@ -468,27 +660,66 @@ func (c *Channel) sender() { } } +// eagerReconnectBaseDelay and eagerReconnectMaxDelay pace the receiver's +// redial loop between failed attempts when eager reconnection is enabled +// (see [NewOutboundChannel]). The delay doubles per failed attempt from the +// base to the cap; the underlying gRPC connection's own dial backoff paces +// actual TCP connection attempts underneath. +const ( + eagerReconnectBaseDelay = 50 * time.Millisecond + eagerReconnectMaxDelay = 2 * time.Second +) + // receiver goroutine receives messages from the stream and routes them to // the appropriate response router. If the stream goes down, it clears the // stream reference and requeues pending requests for retry on a new stream. +// +// With eagerReconnect set, the receiver also re-establishes a lost stream +// itself, with capped exponential backoff, instead of leaving reconnection to +// the sender's next request: a symmetric peer depends on this dialed stream to +// stay registered on its inbound side, so on a node with nothing to send the +// peer would otherwise stall until this node's next request. func (c *Channel) receiver() { + reconnectDelay := eagerReconnectBaseDelay for { stream := c.getStream() if stream == nil { - // Stream not yet available; wait for signal or shutdown - select { - case <-c.streamReady: - // Stream is now available, continue to get it - continue - case <-c.connCtx.Done(): + if !c.eagerReconnect { + // Stream not yet available; wait for signal or shutdown + select { + case <-c.streamReady: + // Stream is now available, continue to get it + continue + case <-c.connCtx.Done(): + // the node's close() method was called: exit receiver goroutine + return + } + } + if c.connCtx.Err() != nil { // the node's close() method was called: exit receiver goroutine return } + if _, err := c.ensureStream(); err != nil { + // The sender records a failed stream creation only when it has + // a request to send. This loop redials on a timer instead, so + // while the caller sends nothing to this node it is the only + // place the peer's unreachability is observed. + c.recordHealth(err) + // Creating the stream failed; pace the next attempt. Do not + // reset the backoff merely because creation later succeeds — a + // stream must demonstrate viability first (see below). + if !c.pauseReconnect(&reconnectDelay) { + return + } + } + continue } + streamStart := time.Now() msg, e := stream.Recv() + // A received frame proves the channel usable; a broken receive condemns it. + c.recordHealth(e) if e != nil { - c.setLastErr(e) // A stale receiver may observe an error after a newer stream has already // replaced this one. Only the goroutine that actually clears the current // stream may requeue pending requests. @@ -500,21 +731,83 @@ func (c *Channel) receiver() { // the node's close() method was called: exit receiver goroutine return } + if c.eagerReconnect { + // A newly created stream that the server immediately rejects + // bypasses the ensureStream error path above: creation succeeds, + // then this Recv fails at once. Pace those redials too, so a + // server that rejects every stream cannot spin this loop. A + // stream that stayed up past the reconnect cap has proven + // viable, so reset the backoff before pacing the next attempt. + if time.Since(streamStart) >= eagerReconnectMaxDelay { + reconnectDelay = eagerReconnectBaseDelay + } + if !c.pauseReconnect(&reconnectDelay) { + return + } + } } else { - // Route to a pending call or dispatch server-initiated back-channel - // requests. Stale (cancelled) calls are silently dropped. - c.router.RouteMessage(c.connCtx, c.id, msg, c.Enqueue) + // A received frame proves the stream is viable: reset the backoff. + reconnectDelay = eagerReconnectBaseDelay + c.dispatchInbound(msg) } } } -func (c *Channel) setLastErr(err error) { +// pauseReconnect waits out the current eager-reconnect backoff delay before the +// next redial attempt, then doubles the delay up to the cap. It returns early +// without growing the delay if the sender re-established the stream during the +// wait, and returns false only when the node closed, signaling the receiver to +// exit. +func (c *Channel) pauseReconnect(delay *time.Duration) bool { + // Drain a stale readiness signal left by our own ensureStream so it cannot + // satisfy the wait instantly; only a signal delivered during the wait — the + // sender re-establishing the stream — should shorten the backoff. + select { + case <-c.streamReady: + default: + } + timer := time.NewTimer(*delay) + defer timer.Stop() + select { + case <-c.streamReady: + // the sender re-established the stream; retry without growing the delay + case <-timer.C: + *delay = min(2*(*delay), eagerReconnectMaxDelay) + case <-c.connCtx.Done(): + return false + } + return true +} + +// dispatchInbound routes one message received by the receiver loop: it delivers +// responses to pending calls and dispatches server-initiated back-channel +// requests to the handler. Stale (cancelled) calls are silently dropped. +// +// A back-channel handler's reply is sent via [Channel.trySend], never the +// blocking [Channel.Enqueue]. The handler runs while holding the router's +// dispatch lock, and this same receiver goroutine must keep reading inbound +// frames; if the reply blocked on a full send queue, the handler would never +// return, the lock would never release, and the receiver would stop making +// progress. +func (c *Channel) dispatchInbound(msg *Message) { + c.router.RouteMessage(c.connCtx, c.id, msg, c.trySend) +} + +// recordHealth records the outcome of a stream operation as this channel's +// [Channel.LastErr]: a non-nil err replaces it, a nil err clears it. Call it +// only for operations that move data, a completed send or a received frame; +// establishing a stream does not prove the channel usable. +func (c *Channel) recordHealth(err error) { c.mu.Lock() defer c.mu.Unlock() c.lastError = err } -// LastErr returns the last error encountered (if any) when using this channel. +// LastErr returns the last error encountered (if any) when using this channel: +// a stream that could not be established, a failed send, or a broken receive. +// It reports the channel's current health, not the outcome of any one request: +// it is last-write-wins across concurrent requests and reverts to nil once +// traffic flows again. func (c *Channel) LastErr() error { c.mu.Lock() defer c.mu.Unlock() diff --git a/internal/stream/channel_test.go b/internal/stream/channel_test.go index eed7c7972..a42571d34 100644 --- a/internal/stream/channel_test.go +++ b/internal/stream/channel_test.go @@ -12,6 +12,7 @@ import ( "github.com/relab/gorums/internal/testutils/mock" "google.golang.org/grpc" + "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials/insecure" ) @@ -20,6 +21,16 @@ const ( streamConnectTimeout = 3 * time.Second ) +// isConnected returns true if the channel has an active stream. +// For outbound channels, also requires the gRPC connection to be in Ready state. +// This method is safe for concurrent use. It is only used by tests. +func (c *Channel) isConnected() bool { + if c.IsInbound() { + return c.connCtx.Err() == nil && c.getStream() != nil + } + return c.conn.GetState() == connectivity.Ready && c.getStream() != nil +} + // testChannel holds the channel and cleanup function. type testChannel struct { *Channel @@ -73,9 +84,43 @@ func holdServer(stream Gorums_NodeStreamServer) error { return nil } +// rejectFirstStreamServer rejects the first stream it accepts and echoes on +// every stream after that, so a channel can record a failure and then recover +// from it on a later stream. +func rejectFirstStreamServer() func(Gorums_NodeStreamServer) error { + var streams atomic.Int32 + return func(stream Gorums_NodeStreamServer) error { + if streams.Add(1) == 1 { + return errors.New("first stream rejected") + } + return echoServer(stream) + } +} + +// waitForLastErr polls until the channel's LastErr matches want (nil or +// non-nil) or the timeout expires, and reports what it observed. +func waitForLastErr(t testing.TB, c *Channel, wantErr bool, what string) { + t.Helper() + deadline := time.Now().Add(defaultTestTimeout) + for time.Now().Before(deadline) { + if (c.LastErr() != nil) == wantErr { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("timeout waiting for %s: LastErr = %v", what, c.LastErr()) +} + // setupChannel creates a channel connected to a server. func setupChannel(t testing.TB, serverFn func(Gorums_NodeStreamServer) error, opts ...grpc.ServerOption) *testChannel { t.Helper() + return setupChannelEager(t, false, serverFn, opts...) +} + +// setupChannelEager is [setupChannel] with control over the channel's eager +// stream reconnection (see [NewOutboundChannel]). +func setupChannelEager(t testing.TB, eagerReconnect bool, serverFn func(Gorums_NodeStreamServer) error, opts ...grpc.ServerOption) *testChannel { + t.Helper() // Start listener lis, err := net.Listen("tcp", "127.0.0.1:0") @@ -101,7 +146,7 @@ func setupChannel(t testing.TB, serverFn func(Gorums_NodeStreamServer) error, op t.Fatalf("failed to dial: %v", err) } - c := NewOutboundChannel(t.Context(), 1, 10, conn, NewMessageRouter()) + c := NewOutboundChannel(t.Context(), 1, 10, conn, NewMessageRouter(), eagerReconnect, nil) tc := &testChannel{ Channel: c, srv: srv, @@ -127,25 +172,30 @@ func (s *mockServer) NodeStream(srv Gorums_NodeStreamServer) error { return s.handler(srv) } -// setupChannelWithoutServer creates a channel that tries to connect to a non-existent server. -func setupChannelWithoutServer(t testing.TB) *testChannel { +// newUnavailableClientConn creates a client connection whose dialer always +// fails, so the connection cannot become ready. +func newUnavailableClientConn(t testing.TB) *grpc.ClientConn { t.Helper() - // Reserve an unused local port, then close the listener to ensure no server is running. - lis, err := net.Listen("tcp", "127.0.0.1:0") + conn, err := grpc.NewClient( + "passthrough:///unavailable", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithContextDialer(func(context.Context, string) (net.Conn, error) { + return nil, errors.New("test connection unavailable") + }), + ) if err != nil { - t.Fatalf("failed to reserve local port: %v", err) - } - addr := lis.Addr().String() - if err := lis.Close(); err != nil { - t.Fatalf("failed to release reserved port: %v", err) - } - // Dial the now-unused address to simulate a missing server. - conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) - if err != nil { - t.Fatalf("failed to dial: %v", err) + t.Fatalf("failed to create unavailable client connection: %v", err) } + t.Cleanup(func() { _ = conn.Close() }) + return conn +} + +// setupChannelWithoutServer creates a channel whose connection cannot reach a server. +func setupChannelWithoutServer(t testing.TB) *testChannel { + t.Helper() + conn := newUnavailableClientConn(t) ctx, cancel := context.WithCancel(context.Background()) - c := NewOutboundChannel(ctx, 1, 10, conn, NewMessageRouter()) + c := NewOutboundChannel(ctx, 1, 10, conn, NewMessageRouter(), false, nil) t.Cleanup(func() { cancel() if err := c.Close(); err != nil { @@ -368,6 +418,75 @@ func TestChannelErrors(t *testing.T) { } } +// TestChannelStreamFailureRecordsLastErr verifies that a request the sender +// cannot deliver because no stream could be established leaves the reason in +// LastErr. LastErr reports node health, so it records the failure whether or +// not the request itself had somewhere to report the error: here a reply with +// no response channel, the one request shape that reaches the sender without +// one. +func TestChannelStreamFailureRecordsLastErr(t *testing.T) { + tc := setupChannelWithoutServer(t) + if err := tc.LastErr(); err != nil { + t.Fatalf("LastErr = %v, want nil before the first request", err) + } + + msg, err := NewMessage(context.Background(), 1, mock.TestMethod, nil) + if err != nil { + t.Fatalf("NewMessage failed: %v", err) + } + tc.Enqueue(Request{Ctx: context.Background(), Msg: msg}) + + deadline := time.Now().Add(defaultTestTimeout) + for tc.LastErr() == nil && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + err = tc.LastErr() + if err == nil { + t.Fatal("LastErr = nil, want the stream creation error for the undelivered request") + } + if !strings.Contains(err.Error(), "connection error") { + t.Errorf("LastErr = %v, want an error containing %q", err, "connection error") + } +} + +// TestChannelLastErrClearsOnRecovery verifies that LastErr reports current +// health rather than history: a channel whose stream failed once reports the +// failure, and reports nil again once traffic flows over a new stream. Without +// clearing, a node with one transient failure would look permanently unhealthy +// and sort behind a node that is down right now. +func TestChannelLastErrClearsOnRecovery(t *testing.T) { + tc := setupChannel(t, rejectFirstStreamServer()) + + // The channel's eager connect creates the stream the server rejects. + waitForLastErr(t, tc.Channel, true, "the rejected stream to be recorded") + + // A completed round trip over the replacement stream proves the channel + // usable again. + if resp := sendRequest(t, tc.Channel, Request{}, 1); resp.Err != nil { + t.Fatalf("sendRequest after recovery: %v", resp.Err) + } + waitForLastErr(t, tc.Channel, false, "LastErr to clear after recovery") +} + +// TestChannelReceiverRecordsStreamFailure verifies that an eager-reconnect +// receiver records its own failed stream creations. The sender records one +// only when it has a request to send, so an idle node would otherwise report +// no error however long it stayed unreachable. +func TestChannelReceiverRecordsStreamFailure(t *testing.T) { + conn := newUnavailableClientConn(t) + ctx, cancel := context.WithCancel(context.Background()) + c := NewOutboundChannel(ctx, 1, 10, conn, NewMessageRouter(), true, nil) + t.Cleanup(func() { + cancel() + if err := c.Close(); err != nil { + t.Errorf("failed to close channel: %v", err) + } + }) + + // No request is ever enqueued: only the receiver's redial loop runs. + waitForLastErr(t, c, true, "the receiver's failed redial to be recorded") +} + // TestChannelEnsureStream verifies that ensureStream correctly manages stream lifecycle. func TestChannelEnsureStream(t *testing.T) { // Helper to prepare a fresh node with no stream @@ -378,7 +497,7 @@ func TestChannelEnsureStream(t *testing.T) { // Extract grpc.ClientConn from existing channel conn := tc.conn // Create new channel with test context without metadata (real implementation captures metadata) - tc.Channel = NewOutboundChannel(t.Context(), tc.id, 10, conn, NewMessageRouter()) + tc.Channel = NewOutboundChannel(t.Context(), tc.id, 10, conn, NewMessageRouter(), false, nil) return tc } @@ -412,7 +531,7 @@ func TestChannelEnsureStream(t *testing.T) { name: "UnconnectedNodeHasNoStream", setup: setupChannelWithoutServer, action: func(tc *testChannel) (BidiStream, BidiStream) { - if err := tc.ensureStream(); err == nil { + if _, err := tc.ensureStream(); err == nil { t.Error("ensureStream succeeded unexpectedly") } if tc.getStream() != nil { @@ -425,7 +544,7 @@ func TestChannelEnsureStream(t *testing.T) { name: "CreatesStreamWhenConnected", setup: newChannelWithoutStream, action: func(tc *testChannel) (BidiStream, BidiStream) { - if err := tc.ensureStream(); err != nil { + if _, err := tc.ensureStream(); err != nil { t.Errorf("ensureStream failed: %v", err) } return tc.getStream(), nil @@ -435,11 +554,11 @@ func TestChannelEnsureStream(t *testing.T) { name: "RepeatedCallsReturnSameStream", setup: newChannelWithoutStream, action: func(tc *testChannel) (BidiStream, BidiStream) { - if err := tc.ensureStream(); err != nil { + if _, err := tc.ensureStream(); err != nil { t.Errorf("first ensureStream failed: %v", err) } first := tc.getStream() - if err := tc.ensureStream(); err != nil { + if _, err := tc.ensureStream(); err != nil { t.Errorf("second ensureStream failed: %v", err) } return first, tc.getStream() @@ -450,12 +569,12 @@ func TestChannelEnsureStream(t *testing.T) { name: "StreamDisconnectionCreatesNewStream", setup: newChannelWithoutStream, action: func(tc *testChannel) (BidiStream, BidiStream) { - if err := tc.ensureStream(); err != nil { + if _, err := tc.ensureStream(); err != nil { t.Errorf("initial ensureStream failed: %v", err) } first := tc.getStream() tc.clearStream(first) - if err := tc.ensureStream(); err != nil { + if _, err := tc.ensureStream(); err != nil { t.Errorf("ensureStream after disconnect failed: %v", err) } return first, tc.getStream() @@ -477,7 +596,7 @@ func TestChannelEnsureStreamAfterBroken(t *testing.T) { tc := setupChannel(t, echoServer) // Ensure we have a stream - if err := tc.ensureStream(); err != nil { + if _, err := tc.ensureStream(); err != nil { t.Fatalf("ensureStream failed: %v", err) } @@ -485,11 +604,112 @@ func TestChannelEnsureStreamAfterBroken(t *testing.T) { tc.clearStream(tc.getStream()) // Ensure we can get it back - if err := tc.ensureStream(); err != nil { + if _, err := tc.ensureStream(); err != nil { t.Fatalf("ensureStream failed after clear: %v", err) } } +// TestChannelEnsureConnectedNodeStreamCancelsAbandonedStream verifies that +// ensureConnectedNodeStream cancels a stream left behind by a previous +// attempt before replacing it with a new one, instead of orphaning it. +// +// Before the fix, when the guard (conn Ready && stream != nil) was false but +// a streamCancel from an earlier attempt was still referenced, +// ensureConnectedNodeStream silently overwrote c.stream and c.streamCancel +// without invoking the previous streamCancel. The abandoned stream then +// stayed alive server-side, and any requests still in flight on it were +// orphaned. +// +// The channel is built directly (bypassing NewOutboundChannel) so no sender +// goroutine runs concurrently and races the manually injected "previous +// attempt" state; ensureConnectedNodeStream is exercised as a plain method +// call, matching how newChannelWithoutStream isolates state in +// TestChannelEnsureStream above. +func TestChannelEnsureConnectedNodeStreamCancelsAbandonedStream(t *testing.T) { + conn := newUnavailableClientConn(t) + if state := conn.GetState(); state == connectivity.Ready { + t.Fatalf("conn state = %v, want anything but Ready", state) + } + + connCtx, connCancel := context.WithCancel(context.Background()) + t.Cleanup(connCancel) + c := &Channel{conn: conn, connCtx: connCtx, connCancel: connCancel} + + // Simulate a stream left behind by a previous ensureConnectedNodeStream + // attempt: a live streamCtx/streamCancel pair and a non-nil stream. + oldCtx, oldCancel := context.WithCancel(connCtx) + c.streamCtx, c.streamCancel = oldCtx, oldCancel + c.stream = newMockBidiStream() + if oldCtx.Err() != nil { + t.Fatal("old stream context should not be cancelled yet") + } + + // conn is not Ready, so the guard is false and ensureConnectedNodeStream + // takes the replace-stream path. + _, _ = c.ensureConnectedNodeStream() + + if oldCtx.Err() == nil { + t.Error("ensureConnectedNodeStream did not cancel the abandoned stream's context before replacing it") + } +} + +func TestChannelCloseCancelsOnlyOwnedPendingRequests(t *testing.T) { + router := NewMessageRouter() + oldStream := newMockBidiStream() + newStream := newMockBidiStream() + t.Cleanup(oldStream.close) + t.Cleanup(newStream.close) + oldChannel := NewInboundChannel(t.Context(), 1, 1, oldStream, router) + newChannel := NewInboundChannel(t.Context(), 1, 1, newStream, router) + t.Cleanup(func() { _ = oldChannel.Close() }) + t.Cleanup(func() { _ = newChannel.Close() }) + + oldReply := make(chan response, 1) + newReply := make(chan response, 1) + oldMessage := Message_builder{MessageSeqNo: ServerSequenceNumber(1), Method: mock.TestMethod}.Build() + newMessage := Message_builder{MessageSeqNo: ServerSequenceNumber(2), Method: mock.TestMethod}.Build() + oldChannel.Enqueue(Request{Ctx: t.Context(), Msg: oldMessage, ResponseChan: oldReply}) + newChannel.Enqueue(Request{Ctx: t.Context(), Msg: newMessage, ResponseChan: newReply}) + + deadline := time.Now().Add(time.Second) + for router.PendingCount() != 2 && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + if got := router.PendingCount(); got != 2 { + t.Fatalf("pending count = %d, want 2", got) + } + + if err := oldChannel.Close(); err != nil { + t.Fatalf("old channel Close: %v", err) + } + select { + case got := <-oldReply: + if !errors.Is(got.Err, ErrNodeClosed) { + t.Fatalf("old request error = %v, want ErrNodeClosed", got.Err) + } + case <-time.After(time.Second): + t.Fatal("old request was not cancelled") + } + select { + case got := <-newReply: + t.Fatalf("new request was cancelled by old channel: %v", got.Err) + default: + } + + newID := newMessage.GetMessageSeqNo() + if !router.deliverPending(newID, response{NodeID: 1, Value: newMessage}) { + t.Fatal("new request was removed from router") + } + select { + case got := <-newReply: + if got.Err != nil { + t.Fatalf("new request response error = %v", got.Err) + } + case <-time.After(time.Second): + t.Fatal("new request did not receive routed response") + } +} + // TestChannelConnectionState verifies connection state detection and behavior. func TestChannelConnectionState(t *testing.T) { tests := []struct { @@ -606,6 +826,241 @@ func TestChannelContext(t *testing.T) { } } +// blockingSendStream blocks every Send until release() is called and blocks +// Recv until the stream is closed. It keeps the channel's sender goroutine +// occupied mid-send so the send queue backs up, simulating a peer that has +// stopped reading (exhausted flow-control windows). Each Send announces its +// message ID on entered when it starts and on sends when it completes, so +// tests can deterministically wait for the sender to be occupied and assert +// FIFO delivery order. +type blockingSendStream struct { + released chan struct{} + closed chan struct{} + entered chan uint64 + sends chan uint64 +} + +func newBlockingSendStream() *blockingSendStream { + return &blockingSendStream{ + released: make(chan struct{}), + closed: make(chan struct{}), + entered: make(chan uint64, 16), + sends: make(chan uint64, 16), + } +} + +func (s *blockingSendStream) Send(msg *Message) error { + s.entered <- msg.GetMessageSeqNo() + select { + case <-s.released: + s.sends <- msg.GetMessageSeqNo() + return nil + case <-s.closed: + return context.Canceled + } +} + +func (s *blockingSendStream) Recv() (*Message, error) { + <-s.closed + return nil, context.Canceled +} + +func (s *blockingSendStream) release() { close(s.released) } +func (s *blockingSendStream) close() { close(s.closed) } + +// waitID waits for an ID on ch (a blockingSendStream signal channel) and +// fails the test if it does not match want or does not arrive in time. +func waitID(t *testing.T, ch <-chan uint64, want uint64, what string) { + t.Helper() + select { + case id := <-ch: + if id != want { + t.Fatalf("%s: message ID = %d, want %d", what, id, want) + } + case <-time.After(defaultTestTimeout): + t.Fatalf("%s: timed out waiting for message %d", what, want) + } +} + +// TestChannelEnqueueRespectsRequestContext verifies that a caller blocked in +// Enqueue on a full send queue is released when its own request context is +// cancelled. Before the fix, Enqueue only watched the channel's connCtx, so a +// worker stuck behind a peer that stopped reading could not be unblocked even +// by a per-call deadline, observed in cluster benchmarks as nodes stalling at +// near-zero throughput for the rest of a run. +func TestChannelEnqueueRespectsRequestContext(t *testing.T) { + stream := newBlockingSendStream() + // Capacity 0: the queue has no slack, so a second request blocks in + // Enqueue as soon as the sender goroutine is occupied in Send. + c := NewInboundChannel(t.Context(), 1, 0, stream, NewMessageRouter()) + t.Cleanup(func() { + stream.close() + _ = c.Close() + }) + + // Occupy the sender: the first request is handed off directly to the + // sender goroutine, whose Send then blocks on the stream. + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + }) + + // The second request cannot be handed off; its Enqueue must block until + // the request's own context is cancelled. + ctx, cancel := context.WithCancel(context.Background()) + reply := make(chan response, 1) + enqueueReturned := make(chan struct{}) + go func() { + defer close(enqueueReturned) + c.Enqueue(Request{ + Ctx: ctx, + Oneway: true, + ResponseChan: reply, + Msg: Message_builder{MessageSeqNo: 2, Method: mock.TestMethod}.Build(), + }) + }() + + // Let the goroutine reach the blocking Enqueue before cancelling. + time.Sleep(20 * time.Millisecond) + cancel() + + select { + case resp := <-reply: + if !errors.Is(resp.Err, context.Canceled) { + t.Errorf("blocked Enqueue reply error = %v, want context.Canceled", resp.Err) + } + case <-time.After(defaultTestTimeout): + t.Fatal("Enqueue ignored request context cancellation; caller is stuck") + } + select { + case <-enqueueReturned: + case <-time.After(defaultTestTimeout): + t.Fatal("Enqueue did not return after request context cancellation") + } +} + +// TestChannelEnqueueTwoWayFailsFastWhenFull verifies that a two-way request +// (one with a waiting local caller) is failed with ErrSendQueueFull instead of +// blocking when the peer's send queue is at capacity, and that requests +// accepted into the queue are still delivered in FIFO order. Quorum calls +// tolerate per-node errors by design, so failing fast lets a call complete +// via the remaining peers instead of stalling the caller behind one peer +// that stopped reading. +func TestChannelEnqueueTwoWayFailsFastWhenFull(t *testing.T) { + stream := newBlockingSendStream() + // Capacity 1: one request occupies the sender, one fills the queue. + c := NewInboundChannel(t.Context(), 1, 1, stream, NewMessageRouter()) + t.Cleanup(func() { + stream.close() + _ = c.Close() + }) + + // Occupy the sender with a one-way request; wait until its Send started. + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + }) + waitID(t, stream.entered, 1, "first send") + + // A two-way request fills the queue's single slot. + reply2 := make(chan response, 1) + c.Enqueue(Request{ + Ctx: context.Background(), + ResponseChan: reply2, + Msg: Message_builder{MessageSeqNo: 2, Method: mock.TestMethod}.Build(), + }) + select { + case resp := <-reply2: + t.Fatalf("second request should be queued, got early reply: %v", resp.Err) + default: + } + + // The next two-way request finds the queue full and must fail fast. + reply3 := make(chan response, 1) + c.Enqueue(Request{ + Ctx: context.Background(), + ResponseChan: reply3, + Msg: Message_builder{MessageSeqNo: 3, Method: mock.TestMethod}.Build(), + }) + select { + case resp := <-reply3: + if !errors.Is(resp.Err, ErrSendQueueFull) { + t.Errorf("full-queue reply error = %v, want ErrSendQueueFull", resp.Err) + } + case <-time.After(defaultTestTimeout): + t.Fatal("two-way Enqueue blocked on a full send queue instead of failing fast") + } + + // FIFO: releasing the stream completes message 1, then message 2 follows. + stream.release() + waitID(t, stream.sends, 1, "first send completion") + waitID(t, stream.sends, 2, "queued send completion") +} + +// TestChannelEnqueueOnewayBlocksWhenFull verifies that one-way requests keep +// today's blocking behavior on a full queue: with no reply to await, +// backpressure is the only mechanism pacing a one-way producer, so a full +// queue must make the producer wait (cancellable via the request context) +// rather than drop the message. +func TestChannelEnqueueOnewayBlocksWhenFull(t *testing.T) { + stream := newBlockingSendStream() + c := NewInboundChannel(t.Context(), 1, 1, stream, NewMessageRouter()) + t.Cleanup(func() { + stream.close() + _ = c.Close() + }) + + // Occupy the sender and fill the queue's single slot. + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + }) + waitID(t, stream.entered, 1, "first send") + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 2, Method: mock.TestMethod}.Build(), + }) + + // The third one-way request must block in Enqueue, not fail. + reply3 := make(chan response, 1) + enqueueReturned := make(chan struct{}) + go func() { + defer close(enqueueReturned) + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + ResponseChan: reply3, + Msg: Message_builder{MessageSeqNo: 3, Method: mock.TestMethod}.Build(), + }) + }() + select { + case resp := <-reply3: + t.Fatalf("one-way Enqueue on a full queue returned early with: %v", resp.Err) + case <-enqueueReturned: + t.Fatal("one-way Enqueue returned without queue space; expected it to block") + case <-time.After(100 * time.Millisecond): + // Still blocked, as intended. + } + + // Releasing the stream drains the queue; the blocked request completes. + stream.release() + select { + case resp := <-reply3: + if resp.Err != nil { + t.Errorf("blocked one-way request failed after release: %v", resp.Err) + } + case <-time.After(defaultTestTimeout): + t.Fatal("blocked one-way request did not complete after queue drained") + } + waitID(t, stream.sends, 1, "first send completion") + waitID(t, stream.sends, 2, "queued send completion") + waitID(t, stream.sends, 3, "unblocked send completion") +} + // TestChannelStreamReadySignaling verifies that the receiver goroutine is properly notified // when a stream becomes available. func TestChannelStreamReadySignaling(t *testing.T) { @@ -706,6 +1161,65 @@ func TestChannelConcurrentStreamReconnect(t *testing.T) { } } +// TestChannelRequestsSurviveStreamChurn verifies that a two-way request +// enqueued while the current stream is concurrently torn down never fails +// with ErrStreamDown. The sender must send on the exact stream its ensure +// step produced: reading the stream again in a separate step races with a +// concurrent clearStream — the receiver observing a broken stream, or a +// cancel watcher — and can observe nil right after a successful ensure, +// failing the request terminally, since a request that was never sent is not +// registered for retry. A request that instead loses the race on Send fails +// with a stream error after registration and is requeued, so under stream +// churn every request must eventually succeed. +func TestChannelRequestsSurviveStreamChurn(t *testing.T) { + tc := setupChannel(t, echoServer) + if !waitForConnection(tc.Channel, streamConnectTimeout) { + t.Fatal("channel should be connected") + } + + // Churn: keep tearing down whatever stream is current, exactly as the + // receiver does when it observes a broken stream (clear, then requeue the + // pending requests stranded on it), forcing every request to race its + // ensure/send steps against a concurrent stream teardown. + churnDone := make(chan struct{}) + go func() { + defer close(churnDone) + for range 2000 { + if s := tc.getStream(); s != nil && tc.clearStream(s) { + tc.requeuePendingMsgs() + } + } + }() + + const concurrency = 8 + var wg sync.WaitGroup + errs := make([]error, concurrency) + for i := range concurrency { + wg.Go(func() { + // Issue requests until the churn ends, recording the first failure. + for msgID := uint64(1); ; msgID++ { + resp := sendRequest(t, tc.Channel, Request{}, uint64(i+1)*100000+msgID) + if resp.Err != nil { + errs[i] = resp.Err + return + } + select { + case <-churnDone: + return + default: + } + } + }) + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Errorf("requester %d: unexpected error during stream churn: %v", i, err) + } + } +} + type recvStartedStream struct { *mockBidiStream started chan struct{} @@ -759,7 +1273,7 @@ func TestChannelStaleReceiverDoesNotRequeueCurrentPending(t *testing.T) { MessageSeqNo: msgID, Method: mock.TestMethod, }.Build() - c.router.Register(msgID, Request{ + c.router.register(c.pendingOwner, msgID, Request{ Ctx: ctx, Msg: msg, ResponseChan: make(chan response, 1), @@ -954,6 +1468,222 @@ func TestChannelLateCancelWatcherRequeuesPending(t *testing.T) { } } +// TestChannelCancelInflightSend verifies the per-request cancel watcher's +// decision logic. The watcher exists solely to unblock a Send stalled by flow +// control (such a Send returns only when its stream dies), so it must clear +// the stream — and requeue the pending requests stranded on it — only while +// the watched Send is still in flight. Once the send has completed, a +// late-running watcher must leave the stream and its pending requests +// untouched: clearing then would sever a healthy stream that later requests +// depend on. A late watcher is routine, not exotic — a caller may cancel its context +// the moment the response arrives (as the benchmark readiness probe does), +// which lands the cancellation between Send returning and the sender's stop +// call, and the watcher goroutine spawned by that cancellation can then run +// arbitrarily late. +func TestChannelCancelInflightSend(t *testing.T) { + tests := []struct { + name string + sendDone bool + wantCleared bool + }{ + // Send still in flight: the watcher must clear the stream to unblock + // it, requeueing the pending request for retry on the next stream. + {name: "InflightSendClearsStream", sendDone: false, wantCleared: true}, + // Send already returned: nothing is blocked, so the watcher must + // leave the healthy stream and its pending requests untouched. + {name: "CompletedSendLeavesStream", sendDone: true, wantCleared: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // The channel is built directly (bypassing the constructors) so no + // sender or receiver goroutine races the manually injected state; + // cancelInflightSend is exercised as a plain method call, matching + // TestChannelEnsureConnectedNodeStreamCancelsAbandonedStream. + connCtx, connCancel := context.WithCancel(context.Background()) + t.Cleanup(connCancel) + c := &Channel{ + sendQ: make(chan Request, 4), + id: 1, + connCtx: connCtx, + connCancel: connCancel, + router: NewMessageRouter(), + pendingOwner: new(pendingOwner), + streamReady: make(chan struct{}, 1), + stream: newLateCancelStream(), + } + + // Register a pending two-way request, as the sender does before Send. + const msgID = 7 + reply := make(chan response, 1) + req := Request{ + Ctx: context.Background(), + Msg: Message_builder{MessageSeqNo: msgID, Method: mock.TestMethod}.Build(), + ResponseChan: reply, + } + c.router.register(c.pendingOwner, msgID, req) + + sendDone := tc.sendDone + c.cancelInflightSend(&sendDone, c.getStream()) + + if gotCleared := c.getStream() == nil; gotCleared != tc.wantCleared { + t.Errorf("stream cleared = %t, want %t", gotCleared, tc.wantCleared) + } + // The pending request must be requeued exactly when the stream was + // cleared; a healthy stream keeps its pending entry for the receiver. + if gotRequeued := len(c.sendQ) == 1; gotRequeued != tc.wantCleared { + t.Errorf("pending request requeued = %t, want %t", gotRequeued, tc.wantCleared) + } + if !tc.wantCleared && !routerExists(c, msgID) { + t.Error("pending request was removed although the stream was left intact") + } + }) + } +} + +// TestChannelCancelImmediatelyAfterSendRecovers exercises the residual window +// in the sender's cancel watcher: a caller that cancels its request context the +// instant its response arrives can land the cancellation between Send returning +// and the sender marking the send done, so a late watcher clears an otherwise +// healthy stream. That clear is self-healing — the stream re-establishes on the +// next send and any requeued request retries — so repeated immediate +// cancellation must never permanently strand the channel. The churn is most +// valuable under -race. It is the end-to-end complement to the decision-table +// coverage in [TestChannelCancelInflightSend]. +func TestChannelCancelImmediatelyAfterSendRecovers(t *testing.T) { + tc := setupChannel(t, echoServer) + if !waitForConnection(tc.Channel, streamConnectTimeout) { + t.Fatal("channel never connected") + } + + // Each iteration cancels the request context the moment the response is in + // hand, maximizing the chance the watcher fires in the post-Send window. + const iterations = 200 + for i := range iterations { + ctx, cancel := context.WithCancel(context.Background()) + reply := make(chan response, 1) + tc.Enqueue(Request{ + Ctx: ctx, + Msg: Message_builder{MessageSeqNo: uint64(i + 1), Method: mock.TestMethod}.Build(), + ResponseChan: reply, + }) + select { + case resp := <-reply: + cancel() // cancel the instant the response arrives + if resp.Err != nil { + t.Fatalf("request %d failed: %v", i+1, resp.Err) + } + case <-time.After(defaultTestTimeout): + cancel() + t.Fatalf("request %d never completed", i+1) + } + } + + // After the churn a fresh request must still complete: a late watcher that + // cleared a healthy stream must not have stranded the channel. + if resp := sendRequest(t, tc.Channel, Request{}, iterations+1); resp.Err != nil { + t.Fatalf("channel stranded after cancel churn: %v", resp.Err) + } +} + +// killFirstStreamServer returns a NodeStream server function that kills the +// first accepted stream immediately — before the client sends anything — and +// serves echo on every later stream. Each accepted stream's ordinal is sent +// on conns, so a test can await the initial stream and the redial. +func killFirstStreamServer() (serverFn func(Gorums_NodeStreamServer) error, conns chan int32) { + var connCount atomic.Int32 + conns = make(chan int32, 4) + serverFn = func(stream Gorums_NodeStreamServer) error { + n := connCount.Add(1) + conns <- n + if n == 1 { + return errors.New("stream killed by test server") + } + return echoServer(stream) + } + return serverFn, conns +} + +// TestChannelEagerReconnectRedialsWithoutSends verifies that a channel with +// eager reconnection re-establishes a stream the server killed without any +// local send prompting it, and that the replacement stream then carries a +// request round trip. A symmetric peer depends on this stream staying +// registered on its inbound side, so waiting for the next local send — the +// default — would leave that peer dropped indefinitely on a node with nothing +// to send. +func TestChannelEagerReconnectRedialsWithoutSends(t *testing.T) { + serverFn, conns := killFirstStreamServer() + tc := setupChannelEager(t, true, serverFn) + + // The sender's initial eager connect creates the first stream with no + // request enqueued; the server kills it on arrival. + select { + case n := <-conns: + if n != 1 { + t.Fatalf("first accepted stream ordinal = %d, want 1", n) + } + case <-time.After(defaultTestTimeout): + t.Fatal("initial stream never reached the server") + } + + // The channel must redial on its own: no Enqueue happens until the + // replacement stream is observed server-side. + select { + case n := <-conns: + if n != 2 { + t.Fatalf("redialed stream ordinal = %d, want 2", n) + } + case <-time.After(defaultTestTimeout): + t.Fatal("channel did not redial the killed stream without a local send") + } + + // The replacement stream must carry a request round trip. + replyCh := make(chan response, 1) + tc.Enqueue(Request{ + Ctx: t.Context(), + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + ResponseChan: replyCh, + }) + select { + case resp := <-replyCh: + if resp.Err != nil { + t.Fatalf("echo over redialed stream failed: %v", resp.Err) + } + case <-time.After(defaultTestTimeout): + t.Fatal("echo over redialed stream never completed") + } +} + +// rejectEveryStreamServer returns a NodeStream server function that rejects +// every accepted stream immediately and counts how many streams it accepted. +func rejectEveryStreamServer() (serverFn func(Gorums_NodeStreamServer) error, count *atomic.Int32) { + count = new(atomic.Int32) + serverFn = func(Gorums_NodeStreamServer) error { + count.Add(1) + return errors.New("stream rejected by test server") + } + return serverFn, count +} + +// TestChannelEagerReconnectBacksOffRejectedStreams verifies that a channel with +// eager reconnection paces its redials with capped backoff when the server +// rejects every stream, instead of spinning and creating a new stream per +// iteration. Without backoff the loop produced thousands of accepted streams +// (5,127 in 250 ms during review); with backoff (50 ms base, doubling to a 2 s +// cap) only a handful of attempts fit in the window below. +func TestChannelEagerReconnectBacksOffRejectedStreams(t *testing.T) { + serverFn, count := rejectEveryStreamServer() + setupChannelEager(t, true, serverFn) + + const window = 500 * time.Millisecond + time.Sleep(window) + // Attempts within the window land at roughly 0, 50, 150, 350 ms plus the + // sender's initial eager connect: about five. The generous bound tolerates + // scheduler jitter while still catching an unpaced spin. + if got := count.Load(); got > 20 { + t.Fatalf("server accepted %d streams in %v; eager reconnect is not backing off (want <= 20)", got, window) + } +} + type signalingRequestHandler struct { called chan *Message } @@ -1252,7 +1982,7 @@ func TestChannelClearStreamDeadlock(t *testing.T) { if err != nil { t.Fatalf("failed to dial: %v", err) } - c := NewOutboundChannel(t.Context(), 1, sendBufSize, conn, NewMessageRouter()) + c := NewOutboundChannel(t.Context(), 1, sendBufSize, conn, NewMessageRouter(), false, nil) t.Cleanup(func() { if closeErr := c.Close(); closeErr != nil { t.Errorf("failed to close channel: %v", closeErr) @@ -1282,7 +2012,7 @@ func TestChannelClearStreamDeadlock(t *testing.T) { if msgErr != nil { t.Fatalf("NewMessage failed: %v", msgErr) } - c.router.Register(uint64(1000+i), Request{ + c.router.register(c.pendingOwner, uint64(1000+i), Request{ Ctx: ctx, Msg: msg, Streaming: false, diff --git a/internal/stream/gorums_message.go b/internal/stream/gorums_message.go index b39269ad6..678756191 100644 --- a/internal/stream/gorums_message.go +++ b/internal/stream/gorums_message.go @@ -21,6 +21,10 @@ func NewMessage(ctx context.Context, msgID uint64, method string, msg proto.Mess if err != nil { return nil, err } + return newMessageFromPayload(ctx, msgID, method, payload), nil +} + +func newMessageFromPayload(ctx context.Context, msgID uint64, method string, payload []byte) *Message { msgBuilder := Message_builder{ MessageSeqNo: msgID, Method: method, @@ -33,7 +37,14 @@ func NewMessage(ctx context.Context, msgID uint64, method string, msg proto.Mess msgBuilder.Entry = append(msgBuilder.Entry, entry) } } - return msgBuilder.Build(), nil + return msgBuilder.Build() +} + +// NewMessageFromPayload creates a [Message] from already-marshaled protobuf +// payload bytes. It is useful when the same request body is sent to many nodes +// but each node requires a different message sequence number. +func NewMessageFromPayload(ctx context.Context, msgID uint64, method string, payload []byte) *Message { + return newMessageFromPayload(ctx, msgID, method, payload) } // AppendToIncomingContext appends client-specific metadata from the [Message] proto message diff --git a/internal/stream/gorums_message_test.go b/internal/stream/gorums_message_test.go new file mode 100644 index 000000000..be00b144e --- /dev/null +++ b/internal/stream/gorums_message_test.go @@ -0,0 +1,41 @@ +package stream + +import ( + "context" + "testing" + + "google.golang.org/grpc/metadata" +) + +func TestMessageConstructorsPreservePayloadAndMetadata(t *testing.T) { + ctx := metadata.NewOutgoingContext(t.Context(), metadata.Pairs("x-request-id", "42", "x-role", "replica")) + payload := []byte("payload") + + fromProto, err := NewMessage(ctx, 7, "test.Method", nil) + if err != nil { + t.Fatalf("NewMessage: %v", err) + } + fromPayload := NewMessageFromPayload(ctx, 8, "test.Method", payload) + + if got := fromProto.GetPayload(); len(got) != 0 { + t.Fatalf("NewMessage payload = %q, want empty payload", got) + } + if got := string(fromPayload.GetPayload()); got != string(payload) { + t.Fatalf("NewMessageFromPayload payload = %q, want %q", got, payload) + } + for name, msg := range map[string]*Message{"proto": fromProto, "payload": fromPayload} { + t.Run(name, func(t *testing.T) { + got := msg.AppendToIncomingContext(context.Background()) + md, ok := metadata.FromIncomingContext(got) + if !ok { + t.Fatal("missing incoming metadata") + } + if values := md.Get("x-request-id"); len(values) != 1 || values[0] != "42" { + t.Fatalf("x-request-id = %v, want [42]", values) + } + if values := md.Get("x-role"); len(values) != 1 || values[0] != "replica" { + t.Fatalf("x-role = %v, want [replica]", values) + } + }) + } +} diff --git a/internal/stream/router.go b/internal/stream/router.go index 2f4a5a429..54227b695 100644 --- a/internal/stream/router.go +++ b/internal/stream/router.go @@ -31,6 +31,24 @@ type RequestHandler interface { HandleRequest(ctx context.Context, msg *Message, release func(), send func(*Message)) } +// pendingOwner is an opaque identity token recording which channel registered +// a pending call. Each channel allocates one token and tags its registrations +// with it, so that closing or requeueing a retired channel affects only that +// channel's calls and never those of its replacement on the same router. +// The struct must not be zero-sized: Go gives distinct zero-size allocations +// the same address, which would make separate tokens compare equal; the +// padding byte guarantees each token a unique address. +type pendingOwner struct { + _ byte +} + +// pendingRequest is a router map entry: a pending call plus the owner token +// of the channel that sent it (nil when registered via the exported Register). +type pendingRequest struct { + request Request + owner *pendingOwner +} + // MessageRouter handles response routing for pending calls on a bidi stream. // It is owned by the Node and injected into each Channel, so the router // survives channel replacement (e.g., inbound reconnects). @@ -44,12 +62,12 @@ type RequestHandler interface { // reference, so handlers registered once are visible to all routers. type MessageRouter struct { mu sync.Mutex - pending map[uint64]Request + pending map[uint64]pendingRequest latency time.Duration handler RequestHandler // shared by reference; may be nil - // localMu serializes in-process handler dispatch, mirroring NodeStream's - // lock+release pattern so local and remote nodes behave identically. - localMu sync.Mutex + // dispatchMu serializes handler dispatch when no stream-owned ordering lock + // exists, covering local and client-side back-channel requests. + dispatchMu sync.Mutex } // NewMessageRouter creates a new MessageRouter with an optional RequestHandler. @@ -60,24 +78,12 @@ type MessageRouter struct { func NewMessageRouter(handler ...RequestHandler) *MessageRouter { handler = append(handler, nil) // ensure handler[0] is always valid return &MessageRouter{ - pending: make(map[uint64]Request), + pending: make(map[uint64]pendingRequest), latency: -1 * time.Second, handler: handler[0], } } -// NewMessageRouterWithLatency creates a new MessageRouter with an initial latency -// for testing. The latency may be updated by subsequent message routing operations. -// This function should only be used in tests. -// -// To change the latency after creation, use [MessageRouter.SetLatency]. -func NewMessageRouterWithLatency(latency time.Duration) *MessageRouter { - return &MessageRouter{ - pending: make(map[uint64]Request), - latency: latency, - } -} - // SetLatency directly sets the latency estimate. This function should only // be used in tests to simulate latency changes without actual message routing. func (r *MessageRouter) SetLatency(latency time.Duration) { @@ -96,18 +102,19 @@ func (r *MessageRouter) PendingCount() int { // DispatchLocalRequest handles the request in-process for the local node, // bypassing the network. It delivers the request to the registered handler, // serializing execution the same way remote nodes do: the next dispatch is -// blocked until the handler returns or calls [ServerCtx.Release]. +// blocked until the handler returns or invokes the release callback it was +// dispatched with. // // For one-way calls, send-completion is confirmed before the handler runs. // For two-way calls, the response is delivered directly to the caller's // response channel via the send closure. func (r *MessageRouter) DispatchLocalRequest(nodeID uint32, req Request) { if req.Ctx.Err() != nil { - req.replyError(nodeID, req.Ctx.Err()) + req.ReplyError(nodeID, req.Ctx.Err()) return } if r.handler == nil { - req.replyError(nodeID, status.Error(codes.Unimplemented, "no request handler registered")) + req.ReplyError(nodeID, status.Error(codes.Unimplemented, "no request handler registered")) return } // One-way calls: confirm "send" completion before running the handler, @@ -126,11 +133,17 @@ func (r *MessageRouter) DispatchLocalRequest(nodeID uint32, req Request) { req.deliver(response{NodeID: nodeID, Value: msg, Err: msg.ErrorStatus()}) } - r.localMu.Lock() - var once sync.Once - release := func() { once.Do(r.localMu.Unlock) } + r.dispatchSerialized(req.Msg.AppendToIncomingContext(req.Ctx), req.Msg, send) +} - go r.handler.HandleRequest(req.Msg.AppendToIncomingContext(req.Ctx), req.Msg, release, send) +// dispatchSerialized starts a handler while holding the router's dispatch lock. +// The next dispatch blocks until the handler invokes the idempotent release +// callback, matching the ordering contract enforced by NodeStream. +func (r *MessageRouter) dispatchSerialized(ctx context.Context, msg *Message, send func(*Message)) { + r.dispatchMu.Lock() + var once sync.Once + release := func() { once.Do(r.dispatchMu.Unlock) } + go r.handler.HandleRequest(ctx, msg, release, send) } // RouteMessage demultiplexes a message received on the client-side (outbound) stream. @@ -148,7 +161,7 @@ func (r *MessageRouter) RouteMessage(ctx context.Context, nodeID uint32, msg *Me send := func(reply *Message) { enqueue(Request{Ctx: ctx, Msg: reply}) } - go r.handler.HandleRequest(ctx, msg, func() {}, send) + r.dispatchSerialized(msg.AppendToIncomingContext(ctx), msg, send) } return } @@ -156,12 +169,18 @@ func (r *MessageRouter) RouteMessage(ctx context.Context, nodeID uint32, msg *Me r.deliverPending(msgID, response{NodeID: nodeID, Value: msg, Err: msg.ErrorStatus()}) } -// Register registers a pending call awaiting a response. -// Called by Channel.sender() after all pre-send checks pass. +// Register registers an unowned pending call awaiting a response. +// Full-router cancellation and requeue operations include unowned calls, +// while channel-scoped operations do not. func (r *MessageRouter) Register(msgID uint64, req Request) { + r.register(nil, msgID, req) +} + +// register associates a pending call with the channel that sent it. +func (r *MessageRouter) register(owner *pendingOwner, msgID uint64, req Request) { req.SendTime = time.Now() r.mu.Lock() - r.pending[msgID] = req + r.pending[msgID] = pendingRequest{request: req, owner: owner} r.mu.Unlock() } @@ -195,13 +214,14 @@ func (r *MessageRouter) RouteInboundMessage(ctx context.Context, nodeID uint32, // may be a no-op if the caller's context is already canceled), false otherwise. func (r *MessageRouter) deliverPending(msgID uint64, resp response) bool { r.mu.Lock() - req, ok := r.pending[msgID] - if ok && !req.Streaming { + pending, ok := r.pending[msgID] + if ok && !pending.request.Streaming { delete(r.pending, msgID) } r.mu.Unlock() if ok { + req := pending.request if resp.Err == nil { r.updateLatency(time.Since(req.SendTime)) } @@ -234,8 +254,23 @@ func (r *MessageRouter) updateLatency(rtt time.Duration) { func (r *MessageRouter) CancelPending() []Request { r.mu.Lock() reqs := make([]Request, 0, len(r.pending)) - for msgID, req := range r.pending { - reqs = append(reqs, req) + for msgID, pending := range r.pending { + reqs = append(reqs, pending.request) + delete(r.pending, msgID) + } + r.mu.Unlock() + return reqs +} + +// cancelPending removes pending requests owned by owner. +func (r *MessageRouter) cancelPending(owner *pendingOwner) []Request { + r.mu.Lock() + reqs := make([]Request, 0) + for msgID, pending := range r.pending { + if pending.owner != owner { + continue + } + reqs = append(reqs, pending.request) delete(r.pending, msgID) } r.mu.Unlock() @@ -252,8 +287,9 @@ func (r *MessageRouter) RequeuePending() (requeue, cancel []Request) { r.mu.Lock() requeue = make([]Request, 0, len(r.pending)) cancel = make([]Request, 0) - for msgID, req := range r.pending { + for msgID, pending := range r.pending { delete(r.pending, msgID) + req := pending.request if req.Streaming { cancel = append(cancel, req) } else { @@ -263,3 +299,21 @@ func (r *MessageRouter) RequeuePending() (requeue, cancel []Request) { r.mu.Unlock() return requeue, cancel } + +// requeuePending removes and classifies pending requests owned by owner. +func (r *MessageRouter) requeuePending(owner *pendingOwner) (requeue, cancel []Request) { + r.mu.Lock() + for msgID, pending := range r.pending { + if pending.owner != owner { + continue + } + delete(r.pending, msgID) + if pending.request.Streaming { + cancel = append(cancel, pending.request) + } else { + requeue = append(requeue, pending.request) + } + } + r.mu.Unlock() + return requeue, cancel +} diff --git a/internal/stream/router_test.go b/internal/stream/router_test.go index 44deb615f..c9e450c40 100644 --- a/internal/stream/router_test.go +++ b/internal/stream/router_test.go @@ -3,11 +3,13 @@ package stream import ( "context" "errors" + "sync" "sync/atomic" "testing" "time" "github.com/relab/gorums/internal/testutils/mock" + "google.golang.org/grpc/metadata" ) func TestRouterRegisterAndDeliver(t *testing.T) { @@ -168,6 +170,29 @@ func TestRouterRequeuePending(t *testing.T) { } } +func TestRouterRequeuePendingByOwner(t *testing.T) { + r := NewMessageRouter() + firstOwner := new(pendingOwner) + secondOwner := new(pendingOwner) + r.register(firstOwner, 1, Request{Ctx: t.Context(), Msg: &Message{}, ResponseChan: make(chan response, 1)}) + r.register(firstOwner, 2, Request{Ctx: t.Context(), Msg: &Message{}, Streaming: true, ResponseChan: make(chan response, 1)}) + r.register(secondOwner, 3, Request{Ctx: t.Context(), Msg: &Message{}, ResponseChan: make(chan response, 1)}) + + requeue, cancel := r.requeuePending(firstOwner) + if len(requeue) != 1 { + t.Fatalf("requeue count = %d, want 1", len(requeue)) + } + if len(cancel) != 1 { + t.Fatalf("cancel count = %d, want 1", len(cancel)) + } + if got := r.PendingCount(); got != 1 { + t.Fatalf("pending count = %d, want 1", got) + } + if !r.deliverPending(3, response{NodeID: 1}) { + t.Fatal("second owner's request was removed") + } +} + // TestRouterRouteInboundMessage verifies RouteInboundMessage demultiplexes // inbound server-side messages: server-initiated IDs (high bit set) are routed // to the pending map; client-initiated IDs (low bit) are dispatched to the handler. @@ -252,6 +277,12 @@ type mockRequestHandler struct { done chan struct{} } +type requestHandlerFunc func(context.Context, *Message, func(), func(*Message)) + +func (f requestHandlerFunc) HandleRequest(ctx context.Context, msg *Message, release func(), send func(*Message)) { + f(ctx, msg, release, send) +} + func newMockRequestHandler() *mockRequestHandler { return &mockRequestHandler{done: make(chan struct{})} } @@ -331,14 +362,14 @@ func TestReplyErrorDoesNotBlockOnCanceledRequest(t *testing.T) { done := make(chan struct{}) go func() { - req.replyError(7, ErrStreamDown) + req.ReplyError(7, ErrStreamDown) close(done) }() select { case <-done: case <-time.After(time.Second): - t.Fatal("replyError blocked on a canceled request with a full reply channel") + t.Fatal("ReplyError blocked on a canceled request with a full reply channel") } } @@ -351,7 +382,7 @@ func TestReplyErrorPrefersDeliveryWhenCanceledAndReplyChanReady(t *testing.T) { } cancel() - req.replyError(7, ErrStreamDown) + req.ReplyError(7, ErrStreamDown) select { case got := <-replyChan: @@ -359,7 +390,7 @@ func TestReplyErrorPrefersDeliveryWhenCanceledAndReplyChanReady(t *testing.T) { t.Fatalf("reply error = %v, want ErrStreamDown", got.Err) } case <-time.After(time.Second): - t.Fatal("replyError dropped a ready delivery on canceled context") + t.Fatal("ReplyError dropped a ready delivery on canceled context") } } @@ -413,6 +444,88 @@ func TestRouterRouteMessage(t *testing.T) { } }) + t.Run("ServerInitiatedIncludesMessageMetadata", func(t *testing.T) { + const ( + key = "request-id" + want = "server-initiated-metadata" + ) + handlerMD := make(chan metadata.MD, 1) + handler := requestHandlerFunc(func(ctx context.Context, _ *Message, release func(), _ func(*Message)) { + defer release() + md, _ := metadata.FromIncomingContext(ctx) + handlerMD <- md + }) + r := NewMessageRouter(handler) + msgCtx := metadata.NewOutgoingContext(connCtx, metadata.Pairs(key, want)) + msg, err := NewMessage(msgCtx, ServerSequenceNumber(1), mock.TestMethod, nil) + if err != nil { + t.Fatalf("NewMessage: %v", err) + } + + r.RouteMessage(connCtx, nodeID, msg, func(Request) {}) + + select { + case md := <-handlerMD: + if got := md.Get(key); len(got) != 1 || got[0] != want { + t.Fatalf("incoming metadata %q = %v, want [%q]", key, got, want) + } + case <-time.After(time.Second): + t.Fatal("handler was not called within timeout") + } + }) + + t.Run("ServerInitiatedPreservesHandlerOrder", func(t *testing.T) { + firstStarted := make(chan struct{}) + secondStarted := make(chan struct{}) + releaseFirst := make(chan struct{}) + var releaseOnce sync.Once + releaseHandler := func() { releaseOnce.Do(func() { close(releaseFirst) }) } + t.Cleanup(releaseHandler) + handler := requestHandlerFunc(func(_ context.Context, msg *Message, release func(), _ func(*Message)) { + switch msg.GetMessageSeqNo() { + case ServerSequenceNumber(1): + close(firstStarted) + <-releaseFirst + case ServerSequenceNumber(2): + close(secondStarted) + } + release() + }) + r := NewMessageRouter(handler) + first := Message_builder{MessageSeqNo: ServerSequenceNumber(1), Method: mock.TestMethod}.Build() + second := Message_builder{MessageSeqNo: ServerSequenceNumber(2), Method: mock.TestMethod}.Build() + + r.RouteMessage(connCtx, nodeID, first, func(Request) {}) + select { + case <-firstStarted: + case <-time.After(time.Second): + t.Fatal("first handler was not called within timeout") + } + + secondReturned := make(chan struct{}) + go func() { + defer close(secondReturned) + r.RouteMessage(connCtx, nodeID, second, func(Request) {}) + }() + select { + case <-secondStarted: + t.Fatal("second handler started before first handler released") + case <-time.After(50 * time.Millisecond): + } + + releaseHandler() + select { + case <-secondStarted: + case <-time.After(time.Second): + t.Fatal("second handler did not start after first handler released") + } + select { + case <-secondReturned: + case <-time.After(time.Second): + t.Fatal("second RouteMessage did not return after dispatch") + } + }) + t.Run("ServerInitiatedNoHandlerIsSilentlyDropped", func(_ *testing.T) { r := NewMessageRouter() msg := Message_builder{MessageSeqNo: ServerSequenceNumber(1), Method: mock.TestMethod}.Build() diff --git a/internal/stream/server.go b/internal/stream/server.go index ff586397d..2106a1f45 100644 --- a/internal/stream/server.go +++ b/internal/stream/server.go @@ -12,7 +12,8 @@ type PeerAcceptor interface { } // PeerNode represents a peer from the perspective of stream dispatch. -// It is implemented by Node and nilPeerNode in the gorums package. +// It is implemented in the gorums package, by Node for an identified peer and +// by nilPeerNode for a connection whose peer ID is not known. type PeerNode interface { // RouteInbound handles a message received from the peer. // Messages with a server-initiated ID (high bit set) are responses to @@ -22,7 +23,13 @@ type PeerNode interface { // release is always called — immediately for server-initiated messages, // or by the handler for client-initiated requests. RouteInbound(ctx context.Context, msg *Message, release func(), send func(*Message)) - Enqueue(req Request) + // TrySend delivers a reply to the peer without blocking on a full send + // queue: see [Server.NodeStream] for why a handler's reply must never be + // able to block here. An implementation with no queue to fail fast against + // may still block on the underlying transport; that is safe as long as it + // only stalls this one peer's connection, not a lock other connections + // depend on. + TrySend(req Request) } // Server handles NodeStream connections. @@ -44,6 +51,13 @@ func NewServer(buffer uint, onConnect func(context.Context), acceptor PeerAccept // NodeStream handles a connection to a single client. The stream is aborted if there // is any error with sending or receiving. +// +// The goroutine below delivers each handler's reply via TrySend, not the +// blocking Enqueue. A handler holds mut until it returns, and it returns only +// once this goroutine takes its reply off finished. If TrySend could block, +// this goroutine would stop draining finished, the handler would never +// return, mut would never unlock, and the Recv loop below could never read +// the next inbound frame: the connection would deadlock. func (s *Server) NodeStream(srv Gorums_NodeStreamServer) error { var mut sync.Mutex // used to achieve mutex between request handlers finished := make(chan *Message, s.buffer) @@ -65,7 +79,7 @@ func (s *Server) NodeStream(srv Gorums_NodeStreamServer) error { case <-ctx.Done(): return case streamOut := <-finished: - peerNode.Enqueue(Request{Ctx: ctx, Msg: streamOut}) + peerNode.TrySend(Request{Ctx: ctx, Msg: streamOut}) } } }() diff --git a/internal/stream/teardown_deadlock_test.go b/internal/stream/teardown_deadlock_test.go new file mode 100644 index 000000000..608067ea6 --- /dev/null +++ b/internal/stream/teardown_deadlock_test.go @@ -0,0 +1,457 @@ +package stream + +import ( + "context" + "errors" + "sync" + "testing" + "testing/synctest" + "time" + + "github.com/relab/gorums/internal/testutils/mock" + "google.golang.org/grpc/metadata" +) + +// TestReceiverDispatchNotWedgedByReentrantReply reproduces the back-channel +// teardown deadlock: a server-initiated (back-channel) request is dispatched to +// a handler while the router's dispatch lock is held; the handler replies on the +// same channel, whose send queue is full because the transport is not draining. +// If that reply enqueue blocks, the handler never releases the dispatch lock and +// the receiver can no longer dispatch inbound frames — the channel deadlocks. +// +// The fix routes back-channel replies through the non-blocking [Channel.trySend] +// (see [Channel.dispatchInbound]). With the fix the handler's reply fails fast, +// the handler returns, the dispatch lock is freed, and the next request +// dispatches. Reverting dispatchInbound to use the blocking [Channel.Enqueue] +// makes this test fail: the dispatch lock is never released. +// +// This is asserted directly on the dispatch lock rather than via synctest's +// all-goroutines-durably-blocked deadlock detection, because a goroutine waiting +// on sync.Mutex.Lock is not "durably blocked" (see testing/synctest); the mutex +// hand-off at the heart of this deadlock is therefore invisible to that +// detection. synctest.Wait is used only to reach a settled state before the +// assertion. +func TestReceiverDispatchNotWedgedByReentrantReply(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + const nodeID = uint32(1) + + // The handler answers every request on the same channel it was + // dispatched on: the reentrant back-channel reply. + dispatched := make(chan uint64, 8) + handler := requestHandlerFunc(func(_ context.Context, msg *Message, release func(), send func(*Message)) { + defer release() + dispatched <- msg.GetMessageSeqNo() + send(Message_builder{MessageSeqNo: msg.GetMessageSeqNo(), Method: mock.TestMethod}.Build()) + }) + r := NewMessageRouter(handler) + + // Capacity 0: once the sender goroutine is occupied in Send, the queue + // has no slack, so a reply would have to wait for space. + stream := newBlockingSendStream() + c := NewInboundChannel(context.Background(), nodeID, 0, stream, r) + defer func() { + // Release the blocked Send and cancel the connection so the sender + // and any goroutine still blocked on the queue can exit before the + // bubble's root returns. + stream.close() + _ = c.Close() + synctest.Wait() + }() + + // Occupy the sender: this one-way request is handed to the sender + // goroutine, which then blocks in Send on a transport that never drains + // (a full or backpressured link during the teardown broadcast). + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + }) + synctest.Wait() // the sender is now durably blocked in Send + + // Dispatch a back-channel request exactly as the receiver loop does. + // dispatchSerialized runs the handler while holding the dispatch lock; + // the handler replies on the same, now-full channel. + first := Message_builder{MessageSeqNo: ServerSequenceNumber(1), Method: mock.TestMethod}.Build() + c.dispatchInbound(first) + synctest.Wait() // let the handler reply and (with the fix) return + + // Invariant: after the handler's reentrant reply, the dispatch lock must + // be free so the receiver can dispatch the next inbound frame. A held + // lock means the reply blocked on the full queue and the loop is wedged. + if !r.dispatchMu.TryLock() { + t.Fatal("dispatch lock still held: a back-channel reply blocked on a full send queue while holding it, deadlocking the receiver's dispatch loop") + } + r.dispatchMu.Unlock() + + // The lock is free: a second back-channel request must still dispatch, + // i.e. the next dispatch is acquired in bounded time. + second := Message_builder{MessageSeqNo: ServerSequenceNumber(2), Method: mock.TestMethod}.Build() + c.dispatchInbound(second) + synctest.Wait() + + got := make(map[uint64]bool) + for { + select { + case id := <-dispatched: + got[id] = true + continue + default: + } + break + } + if !got[ServerSequenceNumber(1)] || !got[ServerSequenceNumber(2)] { + t.Fatalf("dispatched handlers = %v; want both back-channel requests dispatched", got) + } + }) +} + +// TestTrySendDoesNotBlockOnFullQueue is a focused check that the non-blocking +// enqueue used for back-channel replies returns immediately on a full send +// queue even with a background (deadline-free) context, rather than blocking +// unbounded. This is the property that keeps a reply from wedging the receiver: +// unlike the one-way [Channel.Enqueue] path — which blocks until the request +// context is done (see TestChannelEnqueueRespectsRequestContext) — trySend must +// not depend on context cancellation to make progress, because a back-channel +// reply carries only the never-cancelled connection context. +func TestTrySendDoesNotBlockOnFullQueue(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + const nodeID = uint32(1) + stream := newBlockingSendStream() + c := NewInboundChannel(context.Background(), nodeID, 0, stream, NewMessageRouter()) + defer func() { + stream.close() + _ = c.Close() + synctest.Wait() + }() + + // Occupy the sender so the queue is full and cannot drain. + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + }) + synctest.Wait() + + reply := make(chan response, 1) + returned := make(chan struct{}) + go func() { + c.trySend(Request{ + Ctx: context.Background(), + ResponseChan: reply, + Msg: Message_builder{MessageSeqNo: 2, Method: mock.TestMethod}.Build(), + }) + close(returned) + }() + synctest.Wait() + + select { + case <-returned: + default: + t.Fatal("trySend blocked on a full send queue with a background context") + } + select { + case resp := <-reply: + if !errors.Is(resp.Err, ErrSendQueueFull) { + t.Errorf("trySend reply error = %v, want ErrSendQueueFull", resp.Err) + } + default: + t.Fatal("trySend did not fail the request when the queue was full") + } + }) +} + +// TestChannelTrySendDoesNotBlockOnFullQueue is the same check as +// TestTrySendDoesNotBlockOnFullQueue, but against the exported [Channel.TrySend] +// rather than the unexported trySend it wraps. TrySend is the entry point used +// outside this package for replies that must never stall a receive/dispatch +// loop — in particular the drain goroutine in [Server.NodeStream], which hands +// a handler's reply to the peer this way so a stuck or backpressured send +// queue cannot wedge that goroutine (see the invariant in +// TestReceiverDispatchNotWedgedByReentrantReply for the client-side analog). +func TestChannelTrySendDoesNotBlockOnFullQueue(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + const nodeID = uint32(1) + stream := newBlockingSendStream() + c := NewInboundChannel(context.Background(), nodeID, 0, stream, NewMessageRouter()) + defer func() { + stream.close() + _ = c.Close() + synctest.Wait() + }() + + // Occupy the sender so the queue is full and cannot drain. + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + }) + synctest.Wait() + + reply := make(chan response, 1) + returned := make(chan struct{}) + go func() { + c.TrySend(Request{ + Ctx: context.Background(), + ResponseChan: reply, + Msg: Message_builder{MessageSeqNo: 2, Method: mock.TestMethod}.Build(), + }) + close(returned) + }() + synctest.Wait() + + select { + case <-returned: + default: + t.Fatal("TrySend blocked on a full send queue with a background context") + } + select { + case resp := <-reply: + if !errors.Is(resp.Err, ErrSendQueueFull) { + t.Errorf("TrySend reply error = %v, want ErrSendQueueFull", resp.Err) + } + default: + t.Fatal("TrySend did not fail the request when the queue was full") + } + }) +} + +// TestChannelDroppedRepliesCountsOnlyUnreportableDrops verifies that +// DroppedReplies counts a back-channel reply (no ResponseChan) dropped on a +// full queue, but not a two-way request that fails on the same full queue — +// the two-way caller already observes ErrSendQueueFull directly, so counting +// it too would double-report the same failure. +func TestChannelDroppedRepliesCountsOnlyUnreportableDrops(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + const nodeID = uint32(1) + stream := newBlockingSendStream() + c := NewInboundChannel(context.Background(), nodeID, 0, stream, NewMessageRouter()) + defer func() { + stream.close() + _ = c.Close() + synctest.Wait() + }() + + // Occupy the sender so the queue is full and cannot drain. + c.Enqueue(Request{ + Ctx: context.Background(), + Oneway: true, + Msg: Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build(), + }) + synctest.Wait() + + if got := c.DroppedReplies(); got != 0 { + t.Fatalf("DroppedReplies() = %d before any drop, want 0", got) + } + + // A back-channel reply with no ResponseChan: dropped and counted. + c.trySend(Request{ + Ctx: context.Background(), + Msg: Message_builder{MessageSeqNo: 2, Method: mock.TestMethod}.Build(), + }) + synctest.Wait() + if got := c.DroppedReplies(); got != 1 { + t.Errorf("DroppedReplies() = %d after a reply with no ResponseChan, want 1", got) + } + + // A two-way request with a ResponseChan: fails fast but is not counted, + // since the caller already observes ErrSendQueueFull directly. + reply := make(chan response, 1) + c.trySend(Request{ + Ctx: context.Background(), + ResponseChan: reply, + Msg: Message_builder{MessageSeqNo: 3, Method: mock.TestMethod}.Build(), + }) + synctest.Wait() + select { + case resp := <-reply: + if !errors.Is(resp.Err, ErrSendQueueFull) { + t.Errorf("reply error = %v, want ErrSendQueueFull", resp.Err) + } + default: + t.Fatal("two-way request did not fail on the full queue") + } + if got := c.DroppedReplies(); got != 1 { + t.Errorf("DroppedReplies() = %d after a two-way failure, want unchanged at 1", got) + } + }) +} + +// fakeNodeStream is a minimal Gorums_NodeStreamServer for driving +// Server.NodeStream directly: Send blocks until release is called (simulating +// a backpressured or unresponsive link), signaling entered once a Send call +// is in progress; Recv yields messages fed via feed, in the order fed, +// blocking when none are queued. +type fakeNodeStream struct { + ctx context.Context + inbound chan *Message + entered chan struct{} + released chan struct{} + closed chan struct{} + closeOnce sync.Once +} + +func newFakeNodeStream(ctx context.Context) *fakeNodeStream { + return &fakeNodeStream{ + ctx: ctx, + inbound: make(chan *Message, 8), + entered: make(chan struct{}, 1), + released: make(chan struct{}), + closed: make(chan struct{}), + } +} + +func (f *fakeNodeStream) Context() context.Context { return f.ctx } + +func (f *fakeNodeStream) Recv() (*Message, error) { + select { + case m := <-f.inbound: + return m, nil + case <-f.closed: + return nil, context.Canceled + } +} + +func (f *fakeNodeStream) Send(*Message) error { + select { + case f.entered <- struct{}{}: + default: + } + select { + case <-f.released: + return nil + case <-f.closed: + return context.Canceled + } +} + +func (f *fakeNodeStream) feed(m *Message) { f.inbound <- m } +func (f *fakeNodeStream) release() { close(f.released) } +func (f *fakeNodeStream) close() { f.closeOnce.Do(func() { close(f.closed) }) } + +// The remaining methods satisfy grpc.ServerStream; NodeStream never calls them. +func (*fakeNodeStream) SetHeader(metadata.MD) error { return nil } +func (*fakeNodeStream) SendHeader(metadata.MD) error { return nil } +func (*fakeNodeStream) SetTrailer(metadata.MD) {} +func (*fakeNodeStream) SendMsg(any) error { return nil } +func (*fakeNodeStream) RecvMsg(any) error { return nil } + +var _ Gorums_NodeStreamServer = (*fakeNodeStream)(nil) + +// echoOnSameChannelAcceptor is a [PeerAcceptor] whose [PeerNode] replies to +// every inbound request on the same [Channel] it was dispatched from, via +// TrySend — mirroring the real production peerNode adapter — so the +// channel's own stuck sender backs the reply. +type echoOnSameChannelAcceptor struct { + ch *Channel + dispatched chan uint64 +} + +func (a *echoOnSameChannelAcceptor) AcceptPeer(context.Context, BidiStream) (PeerNode, func(), error) { + return echoPeerNode{ch: a.ch, dispatched: a.dispatched}, func() {}, nil +} + +type echoPeerNode struct { + ch *Channel + dispatched chan uint64 +} + +func (p echoPeerNode) RouteInbound(_ context.Context, msg *Message, release func(), send func(*Message)) { + // The reply is produced off the caller's goroutine on purpose: RouteInbound + // runs on the receive loop, which this test requires to stay unblocked. The + // goroutine is the behavior under test, so it cannot move to the caller. + // skipcq: GO-E1007 + go func() { + defer release() + p.dispatched <- msg.GetMessageSeqNo() + send(Message_builder{MessageSeqNo: msg.GetMessageSeqNo(), Method: mock.TestMethod}.Build()) + }() +} + +func (p echoPeerNode) TrySend(req Request) { + p.ch.TrySend(req) +} + +// TestNodeStreamReplyDoesNotWedgeReceiveLoop reproduces the server-side half +// of the teardown deadlock directly against [Server.NodeStream], rather than +// against the individual layers TrySend passes through (as the other tests in +// this file do). A handler's reply is handed off via the drain goroutine's +// call to PeerNode.TrySend; this must never block that goroutine, or +// NodeStream's Recv loop below could never read the next inbound frame — see +// the invariant in TestReceiverDispatchNotWedgedByReentrantReply for the +// client-side analog. Reverting echoPeerNode.TrySend to call ch.Enqueue +// instead of ch.TrySend makes this test hang. +// +// This uses real goroutines and wall-clock timeouts rather than synctest: the +// deadlock's key hand-off is NodeStream's own mut, a plain sync.Mutex, and (as +// documented on TestReceiverDispatchNotWedgedByReentrantReply) a goroutine +// blocked on Mutex.Lock is not "durably blocked" to synctest, so a wedged run +// would hang synctest.Wait itself for the real test timeout instead of failing +// with a clear message. +func TestNodeStreamReplyDoesNotWedgeReceiveLoop(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + fs := newFakeNodeStream(ctx) + t.Cleanup(fs.close) + + // Capacity 0: once the sender goroutine is occupied in Send, the queue + // has no slack, so a reply must fail fast rather than wait for space. + ch := NewInboundChannel(ctx, 1, 0, fs, NewMessageRouter()) + t.Cleanup(func() { _ = ch.Close() }) + + dispatched := make(chan uint64, 8) + srv := NewServer(0, nil, &echoOnSameChannelAcceptor{ch: ch, dispatched: dispatched}) + + done := make(chan error, 1) + go func() { done <- srv.NodeStream(fs) }() + + // Occupy the sender: this one-way request is handed to the channel's + // sender goroutine, which then blocks in Send on a transport that never + // drains (a full or backpressured link during a teardown broadcast). + ch.Enqueue(Request{ + Ctx: ctx, + Oneway: true, + Msg: Message_builder{MessageSeqNo: 100, Method: mock.TestMethod}.Build(), + }) + select { + case <-fs.entered: + case <-time.After(2 * time.Second): + t.Fatal("sender never entered Send") + } + + // Feed three inbound requests. Each handler goroutine reports to + // dispatched before calling send, so requests 1 and 2 are reported + // regardless of whether the drain goroutine wedges: NodeStream's mut + // only serializes Recv iterations on release, and release for request 1 + // fires as soon as its reply is handed off to the (unbuffered) finished + // channel — before the drain goroutine's TrySend call on that reply even + // starts. Request 2's own reply hand-off is what actually depends on the + // drain goroutine: it blocks on finished until the drain goroutine loops + // back to receive again, which happens only once its TrySend call for + // request 1's reply returns. If that TrySend call wedges (the bug this + // guards against), request 2's release never fires, mut is never freed + // again, and request 3 — sitting in fs.inbound — is never read by + // NodeStream's Recv loop or dispatched. So request 3 is the one that + // actually exercises the invariant; 1 and 2 only get it there. + fs.feed(Message_builder{MessageSeqNo: 1, Method: mock.TestMethod}.Build()) + fs.feed(Message_builder{MessageSeqNo: 2, Method: mock.TestMethod}.Build()) + fs.feed(Message_builder{MessageSeqNo: 3, Method: mock.TestMethod}.Build()) + + got := make(map[uint64]bool) + for len(got) < 3 { + select { + case id := <-dispatched: + got[id] = true + case <-time.After(2 * time.Second): + t.Fatalf("dispatched = %v; want all three inbound requests dispatched", got) + } + } + + fs.release() + fs.close() // NodeStream's Recv now returns an error and it exits. + select { + case <-done: + case <-time.After(2 * time.Second): + t.Error("NodeStream did not return after the stream closed") + } +} diff --git a/internal/stream/testhelpers.go b/internal/stream/testhelpers.go new file mode 100644 index 000000000..bc29e9113 --- /dev/null +++ b/internal/stream/testhelpers.go @@ -0,0 +1,28 @@ +package stream + +import "time" + +// This file collects exported constructors that exist only to support tests in +// other packages (package stream's own tests use unexported helpers directly). +// They live in a non-test file because Go test files are not importable across +// packages; keeping them here separates them from production code. + +// NewChannelWithState creates a new Channel with a specific state for testing. +// This function should only be used in tests. +func NewChannelWithState(lastErr error) *Channel { + return &Channel{ + lastError: lastErr, + } +} + +// NewMessageRouterWithLatency creates a new MessageRouter with an initial latency +// for testing. The latency may be updated by subsequent message routing operations. +// This function should only be used in tests. +// +// To change the latency after creation, use [MessageRouter.SetLatency]. +func NewMessageRouterWithLatency(latency time.Duration) *MessageRouter { + return &MessageRouter{ + pending: make(map[uint64]pendingRequest), + latency: latency, + } +} diff --git a/mgr.go b/mgr.go index caa0c374b..bd15cc474 100644 --- a/mgr.go +++ b/mgr.go @@ -99,8 +99,19 @@ func (m *outboundManager) newNode(id uint32, addr string) (*Node, error) { Metadata: m.opts.metadata, DialOpts: m.opts.grpcDialOpts, RequestHandler: m.opts.handler, + // When this node belongs to a server that calls its peers, the peer may + // depend on this connection staying registered on its inbound side. If + // it drops while this node has nothing to send, the peer would stall + // waiting for the next local send, so re-establish it eagerly. Plain + // clients reconnect on the next send. + EagerReconnect: m.opts.inboundMgr != nil, Manager: m, } + if im := m.opts.inboundMgr; im != nil && im.isKnown(id) { + // Stream-state changes on a dialed peer feed the server's + // connected-peer view. + opts.StreamState = im.peerStreamChanged + } n, err := newOutboundNode(addr, opts) if err != nil { return nil, err diff --git a/node.go b/node.go index d2424357e..0581bd147 100644 --- a/node.go +++ b/node.go @@ -77,6 +77,14 @@ type nodeOptions struct { DialOpts []grpc.DialOption RequestHandler stream.RequestHandler Manager *outboundManager // owning manager + // EagerReconnect makes the channel's receiver re-establish a lost stream + // on its own, with capped backoff, instead of waiting for the next local + // send. Set for a server that calls its peers, whose dialed stream the + // remote depends on staying registered. + EagerReconnect bool + // StreamState, if non-nil, is notified when this node's stream comes up or + // goes down, so the owning server can update its connected-peer view. + StreamState func(id uint32, up bool) } // newOutboundNode creates a new node using the provided options. It establishes @@ -106,10 +114,19 @@ func newOutboundNode(addr string, opts nodeOptions) (*Node, error) { ctx := metadata.NewOutgoingContext(context.Background(), md) // Create new outbound channel and establish gRPC node stream - n.channel.Store(stream.NewOutboundChannel(ctx, n.id, opts.SendBufferSize, conn, n.router)) + n.channel.Store(stream.NewOutboundChannel(ctx, n.id, opts.SendBufferSize, conn, n.router, opts.EagerReconnect, onStreamChange(opts))) return n, nil } +// onStreamChange adapts the node's StreamState hook to the channel's +// stream-change callback, which reports only up or down. +func onStreamChange(opts nodeOptions) func(up bool) { + if opts.StreamState == nil { + return nil + } + return func(up bool) { opts.StreamState(opts.ID, up) } +} + // newInboundNode creates a Node for a known peer or self without an active // channel. Used by inboundManager at construction time for all configured // peers; the channel is attached when the peer's stream arrives. @@ -178,6 +195,13 @@ func (n *Node) attachStream(streamCtx context.Context, inboundStream stream.Bidi } } +// isUp reports whether this node's channel can currently carry a request: +// it has an attached channel and, for an outbound channel, a live stream. +func (n *Node) isUp() bool { + ch := n.channel.Load() + return ch != nil && ch.StreamUp() +} + // RouteInbound delivers a response to a pending call or dispatches a // client-initiated request to the registered handler. The release // function is always called. @@ -189,13 +213,22 @@ func (n *Node) RouteInbound(ctx context.Context, msg *stream.Message, release fu // Enqueue enqueues a request to this node's channel. // For local channels the channel handles in-process dispatch directly. // If no channel is available, the request is silently dropped. -// This implements the [stream.PeerNode] interface. func (n *Node) Enqueue(req stream.Request) { if ch := n.channel.Load(); ch != nil { ch.Enqueue(req) } } +// TrySend enqueues a request to this node's channel without ever blocking the +// caller; see [stream.Channel.TrySend]. A request for a node without a channel +// is silently dropped. +// This implements the [stream.PeerNode] interface. +func (n *Node) TrySend(req stream.Request) { + if ch := n.channel.Load(); ch != nil { + ch.TrySend(req) + } +} + // close this node. func (n *Node) close() error { if ch := n.channel.Load(); ch != nil { diff --git a/node_test.go b/node_test.go index 409277b70..293817784 100644 --- a/node_test.go +++ b/node_test.go @@ -387,7 +387,7 @@ func BenchmarkNodeEnqueueSend(b *testing.B) { // Wrap the outbound channel in a Node, adding the one atomic.Pointer.Load // that Node.enqueue performs on every dispatch. n := newInboundNode(1, lis.Addr().String(), func() uint64 { return 0 }, nil) - ch := stream.NewOutboundChannel(context.Background(), 1, 10, conn, n.router) + ch := stream.NewOutboundChannel(context.Background(), 1, 10, conn, n.router, false, nil) b.Cleanup(func() { _ = ch.Close() }) n.channel.Store(ch) diff --git a/opts.go b/opts.go index 6db0bef3b..a29605efe 100644 --- a/opts.go +++ b/opts.go @@ -14,21 +14,30 @@ import ( type DialOption func(*dialOptions) type dialOptions struct { - grpcDialOpts []grpc.DialOption - logger *log.Logger - backoff backoff.Config - sendBuffer uint - metadata metadata.MD - handler stream.RequestHandler - localNodeID uint32 // if non-zero, skip setting handler on this node ID - srvOpts []ServerOption // applied only by NewSystem / NewLocalSystems - outboundNodes NodeListOption // applied only by NewSystem + grpcDialOpts []grpc.DialOption + logger *log.Logger + backoff backoff.Config + sendBuffer uint + metadata metadata.MD + handler stream.RequestHandler + localNodeID uint32 // if non-zero, skip setting handler on this node ID + inboundMgr *inboundManager // set by WithBackChannel; enables eager reconnect for symmetric nodes + srvOpts []ServerOption // applied only by NewSystem } +// DefaultSendBufferSize is the per-node send queue capacity used when no +// explicit size is configured. It is both the backlog threshold at which a peer +// that stopped draining sends is treated as failed, since a full queue fails +// two-way requests fast with [ErrSendQueueFull], and the depth to which one-way +// calls dispatched asynchronously can pipeline. The queue is a buffered +// channel, so each node allocates the full capacity whether or not traffic +// flows. +const DefaultSendBufferSize = 4096 + func newDialOptions() dialOptions { return dialOptions{ backoff: backoff.DefaultConfig, - sendBuffer: 0, + sendBuffer: DefaultSendBufferSize, } } @@ -55,11 +64,16 @@ func WithBackoff(backoff backoff.Config) DialOption { } } -// WithSendBufferSize allows for changing the size of the send buffer used by Gorums. -// A larger buffer might achieve higher throughput for asynchronous calltypes, but at -// the cost of latency. +// WithSendBufferSize sets the per-node send queue capacity. A larger buffer +// may achieve higher throughput for asynchronous call types, at the cost of +// latency. Size 0 selects [DefaultSendBufferSize]: capacity 0 is not viable +// under the full-queue fail-fast semantics, since every two-way request +// enqueued while the sender is busy would fail. func WithSendBufferSize(size uint) DialOption { return func(o *dialOptions) { + if size == 0 { + size = DefaultSendBufferSize + } o.sendBuffer = size } } @@ -73,31 +87,40 @@ func WithMetadata(md metadata.MD) DialOption { } } -// WithServer returns a [DialOption] that installs srv as the back-channel request -// handler and includes srv.NodeID() in the outgoing metadata, allowing the remote -// endpoint to route server-initiated requests back over the bidirectional connection. -// This option is intended for use in symmetric peer configurations, where each node -// is both a client and a server. It will panic if srv is nil. +// WithBackChannel returns a [DialOption] that installs srv as the back-channel +// request handler and includes srv.NodeID() in the outgoing metadata, allowing +// the remote endpoint to route server-initiated requests back over the +// bidirectional connection. Use it for a client that must accept calls from the +// servers it dials. It panics if srv is nil. +// +// A server that calls its own peers does not need this option: [WithPeers] +// installs the back channel on the peer [Configuration] it builds. // // NodeID semantics: -// - If srv.NodeID() == 0, the remote will typically treat this connection as an -// anonymous client and track reverse-direction calls via [ServerCtx.ClientConfig]. -// - If srv.NodeID() > 0, the remote will treat this connection as a known peer -// and route requests via [ServerCtx.Config], as in symmetric peer configurations -// (e.g., outbound configs between replicas). -func WithServer(srv *Server) DialOption { +// - If srv.NodeID() == 0, the remote treats this connection as an anonymous +// client and tracks reverse-direction calls via [ServerCtx.ClientConfig]. +// - If srv.NodeID() > 0, the remote treats this connection as a known peer +// and routes requests via [ServerCtx.Config]. +func WithBackChannel(srv *Server) DialOption { if srv == nil { - panic("gorums: WithServer called with nil server") + panic("gorums: WithBackChannel called with nil server") } + return withServer(srv) +} + +// withServer is WithBackChannel without the nil check, for the server's own +// peer configuration, where the server is known to be non-nil. +func withServer(srv *Server) DialOption { return func(o *dialOptions) { o.handler = srv o.localNodeID = srv.NodeID() + o.inboundMgr = srv.inboundManager o.metadata = metadata.Join(o.metadata, metadataWithNodeID(srv.NodeID())) } } // WithServerOptions bundles [ServerOption]s into a [DialOption] for use with -// [NewSystem] and [NewLocalSystems]. It has no effect when passed to [NewConfig]. +// [NewSystem]. It has no effect when passed to [NewConfig]. // Nil options are silently ignored. func WithServerOptions(opts ...ServerOption) DialOption { return func(o *dialOptions) { @@ -108,15 +131,3 @@ func WithServerOptions(opts ...ServerOption) DialOption { } } } - -// WithOutboundNodes wraps a [NodeListOption] as a [DialOption] for use with -// [NewSystem], instructing it to create an outbound [Configuration] for the given -// peers. It has no effect when passed to [NewConfig] or [NewLocalSystems]. -// A nil opt is a no-op. -func WithOutboundNodes(opt NodeListOption) DialOption { - return func(o *dialOptions) { - if opt != nil { - o.outboundNodes = opt - } - } -} diff --git a/opts_test.go b/opts_test.go index 177964118..a295070e6 100644 --- a/opts_test.go +++ b/opts_test.go @@ -8,7 +8,7 @@ import ( // TestWithServerOptionsFiltersNil verifies that WithServerOptions silently drops // nil ServerOptions rather than storing them, which would cause a panic when -// NewSystem or NewLocalSystems later calls NewServer with the collected options. +// NewSystem later calls NewServer with the collected options. func TestWithServerOptionsFiltersNil(t *testing.T) { opts := newDialOptions() WithServerOptions(nil, WithBufferSizes(8, 8), nil)(&opts) diff --git a/scripts/benchmark.sh b/scripts/benchmark.sh deleted file mode 100755 index 6fb2646fa..000000000 --- a/scripts/benchmark.sh +++ /dev/null @@ -1,13 +0,0 @@ -#!/usr/bin/env bash -inventory="$1" -args="${*:2}" - -out=$(ANSIBLE_STDOUT_CALLBACK=debug ansible-playbook -i "$inventory" -e "bench_args='$args'" benchmark.yml) -status=$? - -if [ $status -eq 0 ]; then - echo "$out" | sed -n '/^MSG:/,/^PLAY/p' | sed '1d;$d' -else - echo -e "Error:\n$out" - exit $status -fi diff --git a/scripts/benchmark.yml b/scripts/benchmark.yml deleted file mode 100644 index 27870a0fb..000000000 --- a/scripts/benchmark.yml +++ /dev/null @@ -1,32 +0,0 @@ ---- -- hosts: servers - vars: - srv_port: ":13371" - tasks: - - name: Start servers - shell: $HOME/benchmark --server '{{ srv_port }}' &> /dev/null & - ignore_errors: yes - async: 1000 - poll: 0 - -- hosts: client - vars: - bench_args: "" - srv_port: ":13371" - tasks: - - name: Get IP Addresses - set_fact: nodelist={%for host in groups['servers']%}"{{hostvars[host]['ansible_default_ipv4']['address'] + srv_port }}"{% if not loop.last %},{% endif %}{% endfor %} - ignore_errors: yes - - name: Run benchmark - command: $HOME/benchmark --remotes "{{ nodelist }}" {{ bench_args }} - register: output - ignore_errors: yes - - name: Print results - debug: msg={{ output.stdout_lines | join('\n') }} - -- hosts: servers - gather_facts: no - tasks: - - name: Kill servers - shell: killall benchmark - ignore_errors: yes diff --git a/scripts/deploy.yml b/scripts/deploy.yml deleted file mode 100644 index 9fe8f2113..000000000 --- a/scripts/deploy.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -- hosts: client:servers - tasks: - - name: Upload benchmark client binary - copy: - src: ../cmd/benchmark/benchmark - dest: $HOME/benchmark - mode: 0755 diff --git a/scripts/killall.yml b/scripts/killall.yml deleted file mode 100644 index 795d061d1..000000000 --- a/scripts/killall.yml +++ /dev/null @@ -1,7 +0,0 @@ ---- -- hosts: client:servers - gather_facts: no - tasks: - - name: Kill benchmark - shell: killall -9 benchmark - ignore_errors: yes diff --git a/server.go b/server.go index 340859e9e..442641be3 100644 --- a/server.go +++ b/server.go @@ -2,6 +2,7 @@ package gorums import ( "context" + "fmt" "net" "github.com/relab/gorums/internal/stream" @@ -18,20 +19,28 @@ type serverOptions struct { connectCallback func(context.Context) interceptors []Interceptor // Peer management options - myID uint32 - peerOpt NodeListOption - onConfigChange func(Configuration) + myID uint32 + peerNodes NodeListOption // Peers to track as they connect; set by WithPeers. + onConfigChange func(Configuration) // Callback registered via WithPeerChange. + outboundNodes NodeListOption // Nodes this server calls; set by WithPeers. + outboundDialOpts []DialOption } // ServerOption is used to change settings for the GorumsServer type ServerOption func(*serverOptions) // WithBufferSizes configures the send and receive buffer sizes for the server. -// The receiveSize controls how many messages the server queues before applying -// backpressure. Similarly, sendSize controls how many messages the server queues -// on its per-node channel for outgoing peer messages in the reverse direction. +// The receiveSize is the capacity of the queue carrying finished handler +// responses to the goroutine that writes them back on the stream; it bounds how +// many requests one connection can have in flight. Its default is 0 +// (unbuffered), which lets a connection read its next request only once the +// current handler's response has been picked up. +// +// The sendSize controls the capacity of the server's per-node send queue for +// outgoing peer messages in the reverse direction, with the same full-queue +// semantics as [WithSendBufferSize]: two-way requests fail fast, one-way +// requests and responses block. A sendSize of 0 selects [DefaultSendBufferSize]. // Larger values may increase throughput at the cost of higher latency. -// The default for both is 0 (unbuffered). func WithBufferSizes(receiveSize, sendSize uint) ServerOption { return func(o *serverOptions) { o.recvBufferSize = receiveSize @@ -68,24 +77,35 @@ func WithInterceptors(i ...Interceptor) ServerOption { } } -// WithConfig configures the server to track a fixed set of peer servers. -// When a recognized peer connects, it is included in the [Configuration] -// returned by [Config]. The myID parameter is this server's own NodeID; -// it is always present in the [Config] so that quorum thresholds account -// for the local replica. +// WithPeers configures the server to both track and call a fixed set of peer +// servers. The myID parameter is this server's own node ID; it is always +// present in the peer [Configuration] so that quorum thresholds account for +// the local replica, and calls to it are served in-process without a network +// round-trip. +// +// The server builds the peer [Configuration] itself, available from +// [Server.PeerConfig], applying opts to the connections it establishes. To +// observe which peers are currently reachable, use [Server.ConnectedPeers]. // -// The optional onChange callback is called after each change to the known -// peer [Configuration] (server peer connect or disconnect). It is invoked -// while any internal locks are held, so it must not call [Config] or other -// blocking methods. Use it only to signal or copy; do not perform long -// work inside the callback. -func WithConfig(myID uint32, opt NodeListOption, onChange ...func(Configuration)) ServerOption { +// The returned option only records the peer set; the [NewServer] call that +// receives it panics if the node source is invalid, for example if it contains +// a duplicate or malformed address. +func WithPeers(myID uint32, nodes NodeListOption, opts ...DialOption) ServerOption { return func(o *serverOptions) { o.myID = myID - o.peerOpt = opt - if len(onChange) > 0 { - o.onConfigChange = onChange[0] - } + o.peerNodes = nodes + o.outboundNodes = nodes + o.outboundDialOpts = append(o.outboundDialOpts, opts...) + } +} + +// WithPeerChange registers a callback invoked after each change to the peer +// [Configuration] (peer connect or disconnect). The callback runs while +// internal locks are held, so it must not call [Server.ConnectedPeers] or other +// blocking methods; use it only to signal or copy, not for long work. +func WithPeerChange(callback func(Configuration)) ServerOption { + return func(o *serverOptions) { + o.onConfigChange = callback } } @@ -95,23 +115,29 @@ type Server struct { grpcServer *grpc.Server handlers map[string]Handler interceptors []Interceptor + outbound Configuration // peer config built by WithPeers; nil if unused *inboundManager } // NewServer returns a new instance of [Server]. // // The server tracks connected clients that are capable of receiving reverse-direction -// calls from the server; these clients are accessible via [ServerCtx.ClientConfig] -// and [Server.ClientConfig]. If [WithConfig] is provided, the server additionally -// tracks a fixed set of peer servers, which are accessible via [ServerCtx.Config] -// and [Server.Config]. +// calls from the server; these clients are accessible via [ServerCtx.ConnectedClients] +// and [Server.ConnectedClients]. If [WithPeers] is provided, the server additionally +// tracks and calls a fixed set of peer servers, accessible via [Server.PeerConfig] +// and, filtered by reachability, [Server.ConnectedPeers]. // // Panics on configuration errors (invalid addresses, duplicate nodes, etc.) // since these are programmer errors detectable at startup. func NewServer(opts ...ServerOption) *Server { var serverOpts serverOptions for _, opt := range opts { - opt(&serverOpts) + if opt != nil { + opt(&serverOpts) + } + } + if serverOpts.sendBufferSize == 0 { + serverOpts.sendBufferSize = DefaultSendBufferSize } // Allocate s first so it can serve as the selfHandler for the inboundManager. // HandleRequest only accesses s.handlers and s.interceptors, both of which are @@ -123,16 +149,40 @@ func NewServer(opts ...ServerOption) *Server { } s.inboundManager = newInboundManager( serverOpts.myID, - serverOpts.peerOpt, + serverOpts.peerNodes, serverOpts.sendBufferSize, serverOpts.onConfigChange, s, ) s.srv = stream.NewServer(serverOpts.recvBufferSize, serverOpts.connectCallback, s.inboundManager) stream.RegisterGorumsServer(s.grpcServer, s.srv) + if serverOpts.outboundNodes != nil { + cfg, err := s.newPeerConfig(serverOpts.outboundNodes, serverOpts.outboundDialOpts) + if err != nil { + panic(fmt.Sprintf("gorums: invalid peer configuration: %v", err)) + } + s.outbound = cfg + s.inboundManager.setPeerConfig(cfg) + } return s } +// newPeerConfig builds the outbound [Configuration] 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 NodeListOption, dialOpts []DialOption) (Configuration, error) { + opts := append([]DialOption{withServer(s)}, dialOpts...) + return NewConfig(nodes, opts...) +} + +// PeerConfig returns the [Configuration] of the peers configured with +// [WithPeers], or nil if [WithPeers] was not used. Calls on the returned +// configuration reach the peers over connections this server establishes; +// calls on the local node are served in-process. +func (s *Server) PeerConfig() Configuration { + return s.outbound +} + // RegisterHandler registers a request handler for the specified method name. // // This function should only be used by generated code. @@ -195,9 +245,15 @@ func (s *Server) GracefulStop() { s.grpcServer.GracefulStop() } -// Stop stops the server immediately. +// Stop stops the server immediately and releases the resources it owns, +// including the peer [Configuration] built by [WithPeers]. It does not use +// gRPC graceful stop, because one-way methods do not respond and would block +// indefinitely. Stop is safe to call more than once. func (s *Server) Stop() { s.grpcServer.Stop() + if s.outbound != nil { + _ = s.outbound.Close() + } } // compile-time assertion for interface compliance. diff --git a/system.go b/system.go index 287f1b6b4..ebdc0e3ee 100644 --- a/system.go +++ b/system.go @@ -3,7 +3,6 @@ package gorums import ( "context" "errors" - "fmt" "io" "net" ) @@ -14,13 +13,12 @@ type System struct { closers []io.Closer srv *Server lis net.Listener - config Configuration // auto-created outbound config; nil if not set } // NewSystem creates a new Gorums System listening on the specified address. -// Accepts any [DialOption]s. Server options may be passed via [WithServerOptions]. -// If [WithOutboundNodes] is provided, an outbound [Configuration] is created -// automatically and can be accessed via [System.OutboundConfig]. +// Accepts any [DialOption]s. Server options may be passed via [WithServerOptions]; +// pass [WithPeers] there to have the server build a peer [Configuration], +// accessible via [System.OutboundConfig]. func NewSystem(addr string, opts ...DialOption) (*System, error) { dialOpts := newDialOptions() for _, opt := range opts { @@ -30,43 +28,61 @@ func NewSystem(addr string, opts ...DialOption) (*System, error) { if err != nil { return nil, err } - sys := &System{ + return &System{ srv: NewServer(dialOpts.srvOpts...), lis: lis, + }, nil +} + +// localServerOptions accumulates the options [NewLocalSystems] applies to every +// system it creates. +type localServerOptions struct { + serverOpts []ServerOption + dialOpts []DialOption +} + +// LocalServerOption configures [NewLocalSystems]. Use [WithLocalServerOptions] +// and [WithLocalDialOptions] to build one. +type LocalServerOption func(*localServerOptions) + +// WithLocalServerOptions applies opts to every server created by [NewLocalSystems]. +func WithLocalServerOptions(opts ...ServerOption) LocalServerOption { + return func(o *localServerOptions) { + o.serverOpts = append(o.serverOpts, opts...) } - if dialOpts.outboundNodes != nil { - cfg, err := sys.newOutboundConfig(dialOpts.outboundNodes, opts...) - if err != nil { - _ = lis.Close() - return nil, fmt.Errorf("gorums: failed to create outbound config: %w", err) - } - sys.config = cfg - sys.closers = append(sys.closers, cfg) +} + +// WithLocalDialOptions applies opts to every server's peer configuration +// created by [NewLocalSystems]. +func WithLocalDialOptions(opts ...DialOption) LocalServerOption { + return func(o *localServerOptions) { + o.dialOpts = append(o.dialOpts, opts...) } - return sys, nil } // NewLocalSystems creates n Gorums systems listening on random localhost ports. // // Each system is assigned a node ID in the range 1..n and is configured to -// communicate with the others using the generated local node list. An outbound +// communicate with the others using the generated local node list. A peer // [Configuration] is created automatically for each system and is available via // [System.OutboundConfig]. // -// The opts may contain any [DialOption]s. Server options may be passed via -// [WithServerOptions]. [WithOutboundNodes] is ignored by this function since -// the local node list is computed internally. +// Use [WithLocalServerOptions] to add [ServerOption]s to every server, and +// [WithLocalDialOptions] to add [DialOption]s to every server's peer +// connections. // // The returned systems are not started. Call [System.Serve] after registering // any services. The returned stop function stops all systems and should be // called when they are no longer needed. // -// If system creation fails, all resources acquired by this function are -// released before returning the error. -func NewLocalSystems(n int, opts ...DialOption) ([]*System, func(), error) { - dialOpts := newDialOptions() +// If listener allocation fails, all listeners acquired so far are closed before +// returning the error. +func NewLocalSystems(n int, opts ...LocalServerOption) ([]*System, func(), error) { + var localOpts localServerOptions for _, opt := range opts { - opt(&dialOpts) + if opt != nil { + opt(&localOpts) + } } listeners, nodeList, err := allocateListeners(n) if err != nil { @@ -75,21 +91,14 @@ func NewLocalSystems(n int, opts ...DialOption) ([]*System, func(), error) { systems := make([]*System, n) for i := range n { myID := uint32(i + 1) - sysSrvOpts := append([]ServerOption{WithConfig(myID, nodeList)}, dialOpts.srvOpts...) - sys := &System{ + sysSrvOpts := append( + []ServerOption{WithPeers(myID, nodeList, localOpts.dialOpts...)}, + localOpts.serverOpts..., + ) + systems[i] = &System{ srv: NewServer(sysSrvOpts...), lis: listeners[i], } - cfg, err := sys.newOutboundConfig(nodeList, opts...) - if err != nil { - for j := range i { - _ = systems[j].Stop() - } - return nil, nil, fmt.Errorf("gorums: failed to create outbound config for system %d: %w", i+1, err) - } - sys.config = cfg - sys.closers = append(sys.closers, cfg) - systems[i] = sys } stop := func() { for _, sys := range systems { @@ -120,19 +129,10 @@ func allocateListeners(n int) ([]net.Listener, NodeListOption, error) { return listeners, WithNodeList(addrs), nil } -// newOutboundConfig creates an outbound [Configuration] for connecting to peers. -// It always prepends a [WithServer] option so that the remote server can dispatch -// server-initiated requests back through the bidirectional connection, regardless of -// whether this system has peer tracking configured. -func (s *System) newOutboundConfig(nodeList NodeListOption, dialOpts ...DialOption) (Configuration, error) { - return NewConfig(nodeList, append([]DialOption{WithServer(s.srv)}, dialOpts...)...) -} - -// OutboundConfig returns the auto-created outbound [Configuration], or nil if none was created. -// An outbound config is created automatically by [NewLocalSystems] and by [NewSystem] when -// [WithOutboundNodes] is provided. +// OutboundConfig returns the server's peer [Configuration], or nil if the +// server was not configured with [WithPeers]. func (s *System) OutboundConfig() Configuration { - return s.config + return s.srv.PeerConfig() } // Addr returns the address the system is listening on. @@ -140,37 +140,38 @@ func (s *System) Addr() string { return s.lis.Addr().String() } -// Config returns a [Configuration] of all connected known peers, including this node. +// ConnectedPeers returns the currently reachable subset of the server's peer +// [Configuration], including this node. // An empty (non-nil) Configuration is returned if no known peers are connected. // The returned slice is replaced atomically on each connect/disconnect; // thus, retaining a reference to an old configuration is safe. -func (s *System) Config() Configuration { - return s.srv.Config() +func (s *System) ConnectedPeers() Configuration { + return s.srv.ConnectedPeers() } -// ClientConfig returns a [Configuration] of all connected client peers +// ConnectedClients returns a [Configuration] of all connected client peers // that can accept server-initiated requests. // An empty (non-nil) Configuration is returned if no client peers are connected. // The returned slice is replaced atomically on each connect/disconnect; // thus, retaining a reference to an old configuration is safe. -func (s *System) ClientConfig() Configuration { - return s.srv.ClientConfig() +func (s *System) ConnectedClients() Configuration { + return s.srv.ConnectedClients() } -// WaitForConfig blocks until cond returns true for the current known-peer +// WaitForPeers blocks until cond returns true for the current connected-peer // [Configuration], or until ctx is cancelled or the system is stopped. // The condition is checked immediately against the current configuration, // so it may return without blocking if the condition is already satisfied. -func (s *System) WaitForConfig(ctx context.Context, cond func(Configuration) bool) error { - return s.srv.waitForKnownConfig(ctx, cond) +func (s *System) WaitForPeers(ctx context.Context, cond func(Configuration) bool) error { + return s.srv.WaitForPeers(ctx, cond) } -// WaitForClientConfig blocks until cond returns true for the current +// WaitForClients blocks until cond returns true for the current // client-peer [Configuration], or until ctx is cancelled or the system is stopped. // The condition is checked immediately against the current configuration, // so it may return without blocking if the condition is already satisfied. -func (s *System) WaitForClientConfig(ctx context.Context, cond func(Configuration) bool) error { - return s.srv.waitForClientConfig(ctx, cond) +func (s *System) WaitForClients(ctx context.Context, cond func(Configuration) bool) error { + return s.srv.WaitForClients(ctx, cond) } // RegisterService registers the service with the server using the provided register function. @@ -201,7 +202,7 @@ func (s *System) Serve() error { // on the client side will get notified by connection errors. // It is safe to call Stop before [System.Serve] to avoid resource leaks. func (s *System) Stop() (errs error) { - // Unblock any WaitForConfig / WaitForClientConfig callers. + // Unblock any WaitForPeers / WaitForClients callers. s.srv.close() // We cannot use graceful stop here since multicast methods does not // respond to the client, and thus would block indefinitely. diff --git a/system_test.go b/system_test.go index 09455c231..7934e021f 100644 --- a/system_test.go +++ b/system_test.go @@ -113,7 +113,7 @@ func TestSystemStopBeforeServeClosesListener(t *testing.T) { // returned by NewLocalSystems closes all pre-allocated listeners even when none of // the systems has had Serve called yet, so no file descriptors are leaked. func TestNewLocalSystemsStopBeforeServeClosesListeners(t *testing.T) { - systems, stop, err := gorums.NewLocalSystems(3, gorumstest.InsecureDialOptions(t)) + systems, stop, err := gorums.NewLocalSystems(3, gorums.WithLocalDialOptions(gorumstest.InsecureDialOptions(t))) if err != nil { t.Fatalf("NewLocalSystems: %v", err) } @@ -147,12 +147,12 @@ func TestSystemSymmetricConfigurationConnectsAllPeers(t *testing.T) { // Wait for connections to establish for i, sys := range systems { ctx := gorumstest.Context(t, 5*time.Second) - if err := sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + if err := sys.WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == len(systems) }); err != nil { - t.Fatalf("system %d: WaitForConfig: %v", i+1, err) + t.Fatalf("system %d: WaitForPeers: %v", i+1, err) } - if got := sys.Config().Size(); got != len(systems) { + if got := sys.ConnectedPeers().Size(); got != len(systems) { t.Fatalf("system %d config size: %d, expected: %d", i+1, got, len(systems)) } } @@ -177,7 +177,7 @@ func awaitSystemReady(t *testing.T, systems []*gorums.System) { t.Helper() for _, sys := range systems { ctx := gorumstest.Context(t, 5*time.Second) - if err := sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + if err := sys.WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == len(systems) }); err != nil { t.Fatalf("awaitSystemReady: %v", err) @@ -189,7 +189,7 @@ func awaitSystemReady(t *testing.T, systems []*gorums.System) { func awaitClientReady(t *testing.T, sys *gorums.System, n int) { t.Helper() ctx := gorumstest.Context(t, 5*time.Second) - if err := sys.WaitForClientConfig(ctx, func(cfg gorums.Configuration) bool { + if err := sys.WaitForClients(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == n }); err != nil { t.Fatalf("awaitClientReady: %v", err) @@ -217,7 +217,7 @@ func createClientServerSystems(t *testing.T) (*gorums.System, *gorums.Server, go // The client dials the server; WithServer wires up the back-channel dispatcher. nodeList := gorums.WithNodeList([]string{sys.Addr()}) - cfg, err := gorums.NewConfig(nodeList, gorums.WithServer(clientSrv), gorumstest.InsecureDialOptions(t)) + cfg, err := gorums.NewConfig(nodeList, gorums.WithBackChannel(clientSrv), gorumstest.InsecureDialOptions(t)) if err != nil { t.Fatal(err) } @@ -242,17 +242,17 @@ func stringEchoHandler(prefix string) gorums.Handler { func configContext(ctx gorums.ServerCtx, client bool) (*gorums.ConfigContext, error) { if client { - configContext := ctx.ClientConfigContext() - if configContext == nil { - return nil, errors.New("ClientConfigContext: expected non-nil config") + clients := ctx.ConnectedClients() + if len(clients) == 0 { + return nil, errors.New("ConnectedClients: expected at least one client") } - return configContext, nil + return clients.Context(ctx), nil } - configContext := ctx.ConfigContext() - if configContext == nil { - return nil, errors.New("ConfigContext: expected non-nil config") + peers := ctx.PeerConfig() + if len(peers) == 0 { + return nil, errors.New("PeerConfig: expected non-empty peer configuration") } - return configContext, nil + return peers.Context(ctx), nil } // outerChainedHandler returns an outer handler that fans out an inner quorum call @@ -398,7 +398,11 @@ func TestSystemHandlerCanMulticastViaConfig(t *testing.T) { sys.RegisterService(nil, func(srv *gorums.Server) { srv.RegisterHandler(mock.TestMethod, func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { t.Logf("System %d received multicast on %v: %v", i+1, mock.TestMethod, in.Msg) - if cfg := ctx.Config(); cfg != nil && cfg.Size() == 3 { + // Release before the nested multicast: the peer configuration + // includes the local node, whose in-process dispatch waits for + // this handler's dispatch lock. + ctx.Release() + if cfg := ctx.PeerConfig(); cfg.Size() == 3 { err := gorums.Multicast( cfg.Context(t.Context()), pb.String("inner-multicast"), @@ -544,7 +548,7 @@ func TestSystemHandlerCanMulticastViaClientConfig(t *testing.T) { sysServer.RegisterService(nil, func(srv *gorums.Server) { srv.RegisterHandler(mock.TestMethod, func(ctx gorums.ServerCtx, in *gorums.Message) (*gorums.Message, error) { t.Logf("SERVER received multicast: %v", in.Msg) - if cfg := ctx.ClientConfig(); cfg != nil && cfg.Size() == 1 { + if cfg := ctx.ConnectedClients(); cfg != nil && cfg.Size() == 1 { err := gorums.Multicast( cfg.Context(t.Context()), pb.String("inner-call"), @@ -757,16 +761,16 @@ func TestSystemLocalDispatchContentionSlowReplica(t *testing.T) { } } -func TestWaitForConfig(t *testing.T) { +func TestWaitForPeers(t *testing.T) { t.Run("ConditionAlreadyMet", func(t *testing.T) { systems := gorumstest.Systems(t, 3) awaitSystemReady(t, systems) ctx := gorumstest.Context(t, 2*time.Second) - if err := systems[0].WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + if err := systems[0].WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == 3 }); err != nil { - t.Fatalf("WaitForConfig: %v", err) + t.Fatalf("WaitForPeers: %v", err) } }) @@ -774,10 +778,10 @@ func TestWaitForConfig(t *testing.T) { systems := gorumstest.Systems(t, 3) ctx := gorumstest.Context(t, 5*time.Second) - if err := systems[0].WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + if err := systems[0].WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == 3 }); err != nil { - t.Fatalf("WaitForConfig: %v", err) + t.Fatalf("WaitForPeers: %v", err) } }) @@ -791,7 +795,7 @@ func TestWaitForConfig(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond) defer cancel() - err = sys.WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + err = sys.WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == 3 // never true }) if !errors.Is(err, context.DeadlineExceeded) { @@ -808,12 +812,12 @@ func TestWaitForConfig(t *testing.T) { errCh := make(chan error, 1) go func() { - errCh <- sys.WaitForConfig(context.Background(), func(cfg gorums.Configuration) bool { + errCh <- sys.WaitForPeers(context.Background(), func(cfg gorums.Configuration) bool { return cfg.Size() == 3 // never true }) }() - // Give WaitForConfig time to enter the select. + // Give WaitForPeers time to enter the select. time.Sleep(20 * time.Millisecond) _ = sys.Stop() @@ -823,7 +827,7 @@ func TestWaitForConfig(t *testing.T) { t.Fatalf("expected ErrStopped, got: %v", err) } case <-time.After(2 * time.Second): - t.Fatal("WaitForConfig did not return after Stop") + t.Fatal("WaitForPeers did not return after Stop") } }) @@ -835,7 +839,7 @@ func TestWaitForConfig(t *testing.T) { for range waiters { ctx := gorumstest.Context(t, 5*time.Second) go func(ctx context.Context) { - errCh <- systems[0].WaitForConfig(ctx, func(cfg gorums.Configuration) bool { + errCh <- systems[0].WaitForPeers(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == 3 }) }(ctx) @@ -843,7 +847,7 @@ func TestWaitForConfig(t *testing.T) { for range waiters { if err := <-errCh; err != nil { - t.Errorf("WaitForConfig: %v", err) + t.Errorf("WaitForPeers: %v", err) } } }) @@ -852,10 +856,10 @@ func TestWaitForConfig(t *testing.T) { sysServer, _, _ := createClientServerSystems(t) ctx := gorumstest.Context(t, 5*time.Second) - if err := sysServer.WaitForClientConfig(ctx, func(cfg gorums.Configuration) bool { + if err := sysServer.WaitForClients(ctx, func(cfg gorums.Configuration) bool { return cfg.Size() == 1 }); err != nil { - t.Fatalf("WaitForClientConfig: %v", err) + t.Fatalf("WaitForClients: %v", err) } }) }