Skip to content

Commit 6fc345c

Browse files
committed
drpcmanager: end-to-end test for flow-control enablement
The manager already threads drpcstream.Options through newStream, so enabling FlowControl in Options.Stream installs windows on every stream the manager creates (client and server) with no additional wiring. Add an end-to-end test over a real net.Pipe connection between two flow-control-enabled managers: a message larger than the send window completes only if credit grants flow back across the connection, exercising the full stream-level credit loop (gate -> grant-on-dispatch -> apply-grant) through the real read/write paths. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> drpcstream: install per-stream windows from a flow-control option Add Options.FlowControl to enable per-stream flow control. When Enabled, NewWithOptions installs the send window (seeded with StreamWindow as initial credit) and the receive window (HighWater + GrantThreshold), so data writes become credit-gated and grants are emitted -- all driven by configuration rather than injected by tests. This is static, symmetric enablement: both peers must enable it with compatible sizes. Advertise-on-enable (safe under mixed enablement), the manager wiring that turns it on for a whole connection, and the connection-level window come in later commits. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> drpcstream,drpcwire: wire the receive-window grant policy into the read path Install a per-stream recvWindow and drive it from the read/recv path: - HandleFrame intercepts an incoming KindWindowUpdate before the assembler and applies it to the send window. Intercepting early keeps a grant that interleaves with an in-progress message (grants are emitted off the write lock) from disturbing reassembly. - A dispatched KindMessage frame returns credit via recvWindow, emitting a KindWindowUpdate once the accrued amount crosses the threshold and the high-water gate is open. - RawRecv/MsgRecv call recvWindow after dequeuing; consuming can reopen a gate the high-water mark had closed and flush the accrued grant. Grants are control frames, appended without blocking, so emitting them from the reader goroutine is safe. The window is opt-in: with no recvw installed no grants are emitted, and an incoming grant with no send window is ignored, so default behavior is unchanged. Stream-level only; the connection-level receive budget and the enablement path that installs the windows come in later commits. The packet assembler can silently drop an unfinished message when a higher message id supersedes it (a legitimate sequence: a send preempted mid-message, then a cancel). Those bytes were counted into the receive window at dispatch and would inflate buffered forever, wedging the high-water gate shut and deadlocking the sender once its credit ran out. The assembler now records the drop (TakeDiscarded) and HandleFrame releases the bytes as consumed, letting their credit flow back. The release runs last (deferred past the replacement frame's accounting and handlePacket), so the grant decision sees the final buffered state rather than the transient dip, and a terminal packet that supersedes a partial suppresses the grant via termination. emitGrant is also a no-op once the stream has terminated: the ring drains queued messages after close, and returning credit behind the terminal frame would invite the peer to send more doomed data. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> drpcstream: add per-stream receive-window grant policy Add recvWindow, the per-stream receive side of flow control. It tracks buffered bytes (in-progress reassembly plus completed, not-yet-consumed data) and decides when to return credit to the sender: grants are withheld while buffered is at or above the high-water mark, and the accrued returnable credit is otherwise released once it reaches the threshold, coalescing many frames into a single grant. Consuming reopens a gate that the high-water mark had closed. dispatched/consumed return the credit delta the caller should emit as a KindWindowUpdate. Emitting the frame, wiring into the read path (HandleFrame/MsgRecv), and the connection-level receive budget come in later commits. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> drpcstream: gate data-frame sends on the per-stream send window Wire the sendWindow credit gate into rawWriteLocked: a KindMessage frame now acquires len(frame) bytes of per-stream send credit before it is handed to the writer. Control frames (invoke/metadata) bypass the gate, and write stats are recorded only after credit is acquired, next to the WriteFrame they describe. The window is opt-in: a stream has no send window by default, so data writes stay ungated (unlimited) and behavior is unchanged until one is installed. terminate closes the window with the send-side error (sigs.send is first-wins, holding io.EOF when a cancel/error path pre-set it), so a send parked on credit returns the same error as one parked in WriteFrame or a later send. Close and SendError close the send window before touching the write lock, so a send parked on credit wakes and releases the lock instead of stalling them on a grant that may never come. SendError pre-closes with io.EOF to match the send signal it sets moments later; Close pre-closes with termClosed for the same reason. CloseSend is a graceful half-close, so it waits for a parked send to complete rather than aborting it. All three (Close, SendError, CloseSend) wait for the write lock without holding s.mu and re-check stream state after acquiring it: the parked writer is only freed by terminate, whose paths (Cancel/Close/SendError/inbound terminal frames) all need s.mu -- and a uniform write-then-mu order is also what keeps CloseSend and Close/SendError from ABBA-deadlocking against each other. Per-stream only; the connection-level window and the enablement path that installs the window come in later commits. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> drpcstream: add per-stream send-window credit primitive Add sendWindow, the per-stream flow-control credit balance on the sender. acquire spends credit, blocking until enough is available; grant adds credit; close terminates the window. acquire returns early on close (with the close error) or context cancellation (with ctx.Err()), consuming no credit in either case; both are checked before any debit, so a canceled context never consumes credit or lets a frame proceed even when credit is available. Grants are no-revoke: grant takes an unsigned delta (the wire type), so a grant can only ever raise the balance, and the addition saturates at math.MaxInt64 so a large or malicious delta can never wrap it negative. The balance is a signed int64; acquire never drives it below zero, but the enablement layer may debit credit for bytes sent before flow control begins enforcing, leaving it negative, in which case acquire blocks until grants restore it. This is the primitive only -- it is not yet wired into the stream write path (that gate comes in a later commit). Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> drpcwire: add KindWindowUpdate flow-control frame Define the KindWindowUpdate control frame that carries a flow-control credit grant: a varint byte delta in the body, for the stream named by the frame's stream id. Add WindowUpdateFrame/ParseWindowUpdate helpers to build and read it. Stream id 0 is reserved for a possible future connection-level window and is unused in v1. The control bit marks the frame as out-of-band signaling, so it is emitted without blocking on data backpressure. It is not relied on for backward compatibility: an unknown control frame is only dropped after packet assembly, so it is not safely ignorable on an active stream. Instead, grants are only ever sent once flow control is active cluster-wide, so a peer that predates flow control never receives one. ParseWindowUpdate enforces the whole v1 wire contract in one place, since these frames are intercepted before packet assembly and its message-id checks: it accepts only a self-contained control frame (Control and Done set) naming a real stream (id != 0) with a positive delta and no trailing bytes. A non-conforming frame yields ok=false and is dropped by the caller, so a reserved stream-0 update from a future peer is ignored rather than mishandled. This is dormant scaffolding for the flow-control work: nothing emits or acts on the frame yet. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> drpcwire,drpcstream: return the real error from a preempted send A send blocked on backpressure used to return an opaque "interrupted" sentinel from WriteFrame, which the data path then remapped through CheckCancelError. WriteFrame lives in drpcwire and cannot know why a send was stopped, so it could only return a generic error and leave the stream to recover the cause. Pass the stream's send signal to WriteFrame directly and have it return that signal's error when the signal fires. The send signal is the write side's own "stop sending" signal, and terminate() always sets it, so a parked send is woken on every termination and returns send.Err(), which is io.EOF in the cancel and error cases. That is the same error the write loop's guard already returns when send is set before a frame is sent, so the parked and unparked paths now return the same thing. This is why the CheckCancelError remap on the write path is removed. It existed to turn the sentinel into the cancellation cause, but the write path now returns io.EOF, which is what gRPC reports on send; the real cause reaches the caller through the receive side, not the send side. Every cancel sets the send signal under the same lock, so the loop guard returns io.EOF before WriteFrame is even reached, and a send woken while parked returns it too. The only WriteFrame errors left are genuine transport failures, which happen only when the stream was not terminated, so there is no cancel cause to substitute and the raw error is correct. Passing a signal instead of a channel also lets WriteFrame resolve its channel lazily, only when a producer actually parks, so a stream that never hits backpressure no longer allocates that channel on every send. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> drpcstream: make SendCancel abortive The previous commit stops terminal frames from being dropped, but SendCancel can still get stuck behind a backpressured send. SendCancel took the write lock before setting the termination signal, so when an in-flight MsgSend/RawWrite was parked on a full buffer while holding the write lock, SendCancel blocked on that lock and the cancel could not take effect until the buffer drained on its own. Local context cancellation should not have to wait on a congested connection. Make SendCancel abortive, matching gRPC where context cancellation does not guarantee that already-buffered messages are flushed first: - Set the termination signal before taking the write lock. A backpressured send watches term in WriteFrame, so setting it first interrupts that send and releases the lock, letting the cancel preempt it instead of waiting for the buffer to drain. - Let a control-bit frame bypass the buffer cap in WriteFrame. KindCancel carries the control bit, so the cancel frame is appended immediately even past the high-water mark. This overshoots the cap by at most one small control frame per terminating stream, which is bounded. SendError, Close, and CloseSend stay ordered: they hold the write lock across the send so their frame stays behind any in-flight data. Document the abortive-vs-ordered contract on each method. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> drpcstream: don't drop terminal frames under backpressure Bounding the connection write buffer made WriteFrame block a producer when the buffer is at its high-water mark; it takes a cancel channel so the producer can bail out. The stream passed s.sigs.term.Signal() as that cancel channel for terminal frames too. That drops terminal frames under congestion. Close, SendError, and SendCancel all call terminate() first, which sets term, and only then send their frame through sendPacketLocked. Since term is already set, a full buffer makes WriteFrame take the cancel branch immediately and the frame is never enqueued. The remote is left without a clean Close/Error/Cancel exactly when the connection is backed up. Before the buffer was bounded this could not happen because WriteFrame never blocked. Pass nil as the cancel channel for terminal frames so they block until the buffer drains and always reach the wire. The ordered terminal paths (SendError/Close/CloseSend) hold the write lock across the send, so their frame stays behind any in-flight data. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> drpcwire: make the write buffer limit configurable The high-water mark for the mux writer was hardcoded, so callers had no way to raise it for high-throughput connections or lower it under tight memory budgets. Expose it as WriterOptions.MaximumBufferSize and plumb it through drpcmanager.Options.Writer, mirroring how Reader and Stream options already flow through the manager. The default stays at 1 MiB when the option is left unset, so existing behavior is unchanged. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> drpcwire: bound the connection write buffer The receiver side already bounds memory through the stream ring buffer, but the connection-level MuxWriter had no such limit. A producer that serializes frames faster than the network can drain them would let the pending buffer grow without end, so a fast sender over a slow connection could OOM the process. MuxWriter now enforces a high-water mark on the pending buffer. Once it is reached, WriteFrame blocks the calling stream until run drains the buffer onto the wire. This is plain backpressure rather than a full credit-based flow control scheme, which is enough to cap sender memory. The trade-off is that peak memory is up to ~2x the mark, since the in-flight buffer being written coexists with the pending one. Blocking on a full buffer means a write can no longer ignore stream termination, which matters now that multiplexing has many streams sharing one MuxWriter: a single canceled stream must not wait on the shared buffer draining. WriteFrame therefore takes a cancel channel, and the stream passes its term signal so cancel, close, and remote errors all unblock a parked write. The error is mapped back through CheckCancelError like the existing termination checks. Producers parked on backpressure are woken by closing and replacing a drain channel after each flush, which broadcasts to every waiter at once. Stop wakes them too, so shutdown does not depend on the buffer draining even when run is stuck in a slow underlying write. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> drpcstream: introduce shared BufferPool for ring buffer Add a BufferPool backed by sync.Pool that is shared across all streams within a Manager. The ring buffer now obtains a pooled buffer on Enqueue and copies the message into it, instead of growing a fixed slice per slot. Dequeue hands the buffer's data to the consumer and advances the tail immediately, and Done releases the buffer back to the pool once the consumer is finished with it. Keeping the Dequeue/Done contract means the consumer works with a plain []byte and never has to know whether the queue is backed by a pool or by fixed buffers. Because Dequeue advances the tail right away rather than waiting for Done, Close no longer has to block until in-flight buffers are released. The pool is a required parameter in the Stream constructor, created once per Manager and passed to all streams it creates. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> drpcpool: close connections returned to a closed pool Pool.Close only sees the connections currently idle in the cache. Connections that are checked out serving an in-flight Invoke or NewStream live outside the cache and are returned later via Put. Because the pool had no closed state, such a late Put re-inserted the connection and armed a fresh expiration timer, resurrecting the pool and leaking the connection (and its manager goroutines) until the timer fired. This is reachable on shutdown: long-lived streams (e.g. rangefeeds) hold their connection checked out, so Close cannot reach them. When the stream context is later canceled, the connection is handed back via Put and lingers well past the point the pool was closed. Mark the pool closed under the lock in Close, and have Put close the connection immediately instead of caching it once the pool is closed. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> drpc: enable stream multiplexing Squashed result of the upstream stream-multiplexing branch. See the merged PRs for granular history: - #39 drpcmanager: fix race between manageReader and stream creation - #42 *: move frame assembly from reader to stream - #43 *: extract PacketAssembler for frame-to-packet assembly - #44 drpcmanager: replace manageStreams loop with per-stream goroutines - #45 *: use per-stream Finished signal instead of shared sfin channel - #46 drpcmanager: use atomic counter for client stream ID generation - #47 drpcmanager: replace streamBuffer with a streams registry - #51 drpc: enable stream multiplexing A connection now runs multiple concurrent client and server streams over a single transport. Frames carry stream IDs and are interleaved on the wire by a shared MuxWriter. Each stream owns its own packet ring buffer, Finished signal, and goroutine, and the manager tracks live streams in an activeStreams registry. New: drpcwire.MuxWriter, drpcwire.PacketAssembler, drpcstream.ringBuffer, drpcmanager.activeStreams. Removed: the drpccache package, drpcwire/writer (now MuxWriter), drpcstream/pktbuf (now ringBuffer), drpcmanager/streambuf (now activeStreams), drpcmanager.Options.InactivityTimeout, and the drpcconn shared write buffer plus stats infrastructure (CollectStats, Stats, drpcstats wiring). protoc-gen-go-drpc: generate HTTP gateway routes from proto annotations Read `google.api.http` annotations from proto method options and emit `DRPC<Service>GatewayRoutes` functions in the generated `*_drpc.pb.go` files. Each function returns a `[]drpc.HTTPRoute` containing the HTTP method, path, and a reference to the corresponding RPC client method. This lets consumers (like CockroachDB's DRPC gateway) register HTTP routes from generated data instead of maintaining manual route tables that drift out of sync with proto definitions. Streaming RPCs and methods without HTTP annotations are skipped. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> ci: add merge guard workflow for fixup commits and do-not-merge label This guard will add a layer of protection against accidental merge of fixup commits. fixup! drpcpool: add support for ShouldRecord Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> fixup! drpcpool: add support for ShouldRecord fixup! drpcpool: add support for ShouldRecord fixup! drpcpool: add support for ShouldRecord fixup! drpcpool: add support for ShouldRecord drpcpool: add support for ShouldRecord *: plug in no-op metric implementations for nil fields This is a follow-up to PR #33 with the following fixes: - Initialize nil metric fields with no-op implementations at construction time (server, client, pool) so that call sites don't need nil guards. - Unexport MeteredTransport behind a NewMeteredTransport constructor that handles nil counters. *: add drpc client and server metrics Add metrics collection for drpc client and server connections, and connection pools. - Introduce Counter and Gauge metric interfaces in the drpc package, allowing us to plug in the metrics implementation from cockroach - Client metrics: bytes sent/received - Server metrics: bytes sent/received, TLS handshake errors - Pool metrics: pool size, connection hits/misses drpcserver: add TLSConfig and cipher suite restriction options Add TLSConfig and TLSCipherRestrict server options. When TLSConfig is set, Serve() wraps the listener with tls.NewListener and ServeOne() performs an explicit TLS handshake. The TLSCipherRestrict callback, if provided, runs after a successful handshake and rejects connections using a cipher that is not allowed for server use. *: migrate golangci-lint config to v2 golangci-lint v2 changed the config schema: `linters-settings` moved under `linters.settings`, formatters (gofmt, gci, goimports) are no longer linters, and several settings were removed (golint, unused. check-exported, unparam.algo, govet.use-installed-packages). Linters absorbed into staticcheck (gosimple, stylecheck) and removed upstream (rowserrcheck) are dropped from the disable list. The noctx linter now flags tls.Conn.Handshake — switched to HandshakeContext(ctx) so cancellation propagates to the TLS handshake. Also adds a default-configuration tripwire: both directions of a default-options connection are sniffed at the byte level, and the test fails if any KindWindowUpdate reaches the wire -- so no future change in the stack can silently enable flow control by default. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com> *: map conn close to Unavailable (client) and Canceled (server) Introduce drpc.StreamKind (StreamKindClient/StreamKindServer) and add `Kind()` to the drpc.Stream interface so that remote conn close errors are mapped correctly, matching gRPC semantics: - Client streams return `ClosedError` (mapped to `Unavailable` status code). - Server streams return `context.Canceled`. internal/integration: align tests with gRPC status behavior Update integration tests to match the current gRPC-status-based error model. - Replace drpcmetadata.Get with drpcmetadata.GetFromIncomingContext. - Switch error assertions from drpcerr.Code and direct equality to status.FromError plus codes.Code checks. - Update cancel/transport tests to handle wrapped status errors (Canceled and Unavailable). - Adjust stats test byte-count expectations for the new error encoding. - Bump internal/integration to Go 1.25 and update golang.org/x/exp (with corresponding go.sum updates) for trace compatibility. drpchttp: remove drpchttp/ We have DRPC gateway in cockroach, so this package can be removed. Fixes: https://github.com/cockroachdb/cockroach/issues/164479 internal: remove twirpcompat examples: remove examples *: overhaul Makefile and add GitHub action This commit cleans up our fork to focus on our needs by removing storj specific CI and build infrastructure that we don't use. The original Makefile relied heavily on custom wrapper scripts in scripts/ that orchestrated testing across multiple modules and build configurations specific to storj's environment. 1. Deleted Jenkinsfile (Jenkins CI), flake.nix and flake.lock (Nix development environment), and the entire scripts/ directory which contained run.sh, docs.sh, cloc.sh, and other tooling we don't need. 2. Makefile: Rewrote it with direct go commands instead of wrapper scripts. The PKG variable excludes drpchttp/, internal/ (separate test modules), and cmd/. I plan to remove drpchttp/ package and add the cmd/ package back later as I was seeing some errors with it. The build target now compiles all relevant packages for verification. 3. Created .github/workflows/ci.yaml with actions checkout@v4, setup-go@v5, golangci-lint-action@v4 and two parallel jobs: build-and-test runs build/test/vet, while lint runs staticcheck and golangci-lint. The workflow triggers on pushes and PRs to main, using Go 1.25. 4. Fixed and suppressed some lint errors. 5. Disabled failing linters temporarily: Moved errcheck, errorlint, godot, and err113 to the disabled list with TODO comments so they can be re-enabled incrementally. Also cleaned up deprecated linters like exportloopref (replaced by copyloopvar in Go 1.22+), exhaustivestruct, gomnd, ifshort, and interfacer, and fixed golangci-lint config deprecation warnings. The repository is now significantly cleaner with 11 files deleted (including a 9.5MB committed binary that was accidentally tracked) and a straightforward build process. Fixes: cockroachdb/cockroach#163908 Merge pull request #28 from cthumuluru-crdb/drpc-errors *: convert errors to status errors to stay backward compatible with gRPC *: convert errors to status errors to stay backward compatible with gRPC gRPC treats connection errors specially. It uses `ConnectionError` interface to represent failures at the transport level, which are mapped to the `Unavailable` status code. `ConnectionError` can occur in the following situations: - During server transport creation: - [Failure during the server handshake](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_server.go#L149-L165). - [Errors while writing the settings frame](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_server.go#L209-L211). - [Errors while writing the window update frame](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_server.go#L213-L217). - [Failure to set the TCP timeout](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_server.go#L236-L240). - [Errors while reading the client preface or settings frame](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_server.go#L310-L337). - During client connection setup (dialing): - [Failure to dial the connection](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_client.go#L222-L228). - [Failure to set the TCP timeout](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_client.go#L269-L275). - [Errors during the client handshake](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_client.go#L291-L307). - [Errors while writing the settings frame](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_client.go#L422-L430). - [Errors while writing the window update frame](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_client.go#L445-L456). - [Errors while reading frames from the server](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_client.go#L1666-L1691). - [Errors while reading the server preface or settings frames](https://github.com/grpc/grpc-go/blob/49e224f832c21df2cd3fe833189c8a50244f0eee/internal/transport/http2_client.go#L1625-L1636). All errors are ultimately translated into gRPC status codes, which callers use to determine the appropriate next steps. To be compatible with gRPC, this change introduces ConnectionError class and converts it to `Unavailable` status code. drpcmetadata: split incoming and outgoing metadata keys (#23) - Add separate keys and methods for incoming and outgoing metadata. - Renamed methods to be similar to their grpc counterparts - Removed Add and AddPairs (for incoming context) as they don't have grpc counterparts and no existing usage Merge pull request #25 from cthumuluru-crdb/issue-159372 drpcclient: classify connection errors as `Unavailable` drpcmanager: Add GrpcMetadataCompatMode (#26) drpcclient: classify connection errors as Unavailable gRPC classifies connection failures as `Unavailable`. Connection errors include failures during TCP dialing as well as the TLS handshake. For backward compatibility, we mirror gRPC's behavior and return the same status codes to clients. Merge pull request #24 from cthumuluru-crdb/issue-158270 drpcclient: disable soft cancel dial option by default drpcclient: disable soft cancel dial option by default Soft cancel support in drpc allows the underlying transport to remain open when a stream's context is canceled. Soft cancel is enaled by default to reduce connection churn seen with stream pooling. However, after enabling soft cancel, we observed a hang where the server-side reader blocks waiting for the stream buffer to close, an event that only occurs when the manager (also the transport) itself is closed. While we work on a fix for this issue, we are disabling soft cancel usage by default for dialing DRPC connections. ----- goroutine trace blocked waiting for the stream buffer to close: ``` goroutine 3335 [sync.Cond.Wait]: sync.runtime_notifyListWait(0x140060fdae8, 0x0) GOROOT/src/runtime/sema.go:606 +0x140 sync.(*Cond).Wait(0x140060fdad8) GOROOT/src/sync/cond.go:71 +0xa4 storj.io/drpc/drpcmanager.(*streamBuffer).Wait(0x140060fdad0, 0x3) external/io_storj_drpc/drpcmanager/streambuf.go:53 +0xac storj.io/drpc/drpcmanager.(*Manager).manageReader(0x140060fda00) external/io_storj_drpc/drpcmanager/manager.go:299 +0x508 created by storj.io/drpc/drpcmanager.NewWithOptions in goroutine 3332 external/io_storj_drpc/drpcmanager/manager.go:137 +0x460 ``` Merge pull request #22 from suj-krishnan/handle_grpc_metadata drpcconn, drpcmanager: add support to send and receive grpc metadata drpcconn, drpcmanager: add support to send and receive grpc metadata This fix ensures that if the outgoing context has any grpc metadata, it is processed and sent as drpc metadata in the Invoke (unary) and NewStream (stream) flow. On the receiving end, the fix ensures that the server stream that processes an incoming packet duplicates any drpc metadata into the grpc metadata in the incoming context. This will enable us in the short-term to send and receive drpc metadata even while the cockroach code base continues to use the grpc metadata package to populate and receive metadata, when DRPC is enabled. Merge pull request #19 from cthumuluru-crdb/issue-149600 drpcclient: add additional dial options to dial a DRPC conn drpcclient: add additional dial options to dial a drpc conn `drpcclient.ClientConn` is a client connection with interceptor support. It takes a connection so that the interceptor chain can be applied to both direct and pooled connections. This change adds new dialer options `WithContextDialer` and `WithTLSConfig` to simplify the consuming code paths. This change also introduces the `WithContextDialer` and `WithTLSConfig` options to simplify connection dialing logic in the application (aka client) code. drpcwire: replace drpcerr with gRPC status protobuf for error marshaling (#16) In CockroachDB, we rely heavily on gRPC status for returning error messages with codes. This change ensures that our existing error handling continues to work with DRPC without requiring any modifications to the CockroachDB codebase. This is a pragmatic, temporary solution. In the future, when we eliminate gRPC, we can replace this dependency by improving the drpcerr package to achieve feature parity. This change replaces the custom binary error encoding (a simple 8-byte code + message) with the standard protobuf format from gRPC's google.rpc.Status. Additionally, we introduce an ErrorVersion system to future-proof the wire format for potential changes to error serialization. Key changes: - We're now using google.golang.org/grpc/status for error conversion instead of drpcerr. - Errors are marshaled and unmarshaled as protobuf-encoded google.rpc.Status messages. - The change handles both gRPC status errors and standard Go context errors (Canceled, DeadlineExceeded). - Added ErrorVersion enum with version byte prefix to support future serialization format changes. - Wire format is now: [1 byte: version][N bytes: protobuf data]. drpcmetadata: ensure metadata immutability by returning copy (#17) This prevents mutation of metadata maps stored in context by ensuring Get() returns a copy and Add()/AddPairs() work with copies rather than modifying the original map in place. drpc: add TLS certificate handling and metadata infra for auth interceptors (#11) This commit adds infrastructure needed for authentication interceptors: 1. New drpcctx/tlscert.go: Functions to store/retrieve TLS peer certificates in context 2. Server-side TLS certificate extraction in drpcserver 3. Improved metadata API with ClearContext, ClearContextExcept, and GetValue functions 4. Client-side per-RPC metadata support via WithPerRPCMetadata option drpcmux: add tests for server interceptors (#14) This commit adds tests for the server interceptors and RPC handling: - Tests for unary and streaming server interceptors - Validation of correct interceptor chaining and execution order - Error handling and propagation through interceptor chains - Edge cases with empty interceptor chains drpcmux: fix server interceptor selection logic (#13) This commit fixes a bug in the interceptor selection logic in HandleRPC. The issue was that we were passing a stream to the receiver for the case where the input is unitary and the output is a stream. The fix is to receive the message from the stream within the final receiver after going through the stream interceptor pipeline. This also means we no longer receive the message outside the switch statement. drpcmux: remove redundant context parameter from stream handlers (#12) This change simplifies the API by removing the explicit context parameter from StreamHandler and StreamServerInterceptor interfaces since it can be retrieved from the stream itself via stream.Context(). makefile: add gen-bazel target (#15) This change adds a `gen-bazel` target which generates a `WORKSPACE` file and runs gazelle to generate `BUILD.bazel` files. Generating these files allows a local clone of the repository to be used directly when building cockroach using the `override_repository` flag, for example: ``` ./dev build short -- --override_repository=io_storj_drpc=${CURDIR} ``` The new files are added to `.gitignore` so that generating them doesn't interfere with commits. This also adds a `clean-bazel` target that cleans up these files. Inspired by: https://github.com/cockroachdb/pebble/commit/cab9f08de0db22259b8c58ebc0980aec7b7eaae0 drpcmux: add support for server interceptors (#10) This change introduces support for server-side interceptors in DRPC. For a deeper understanding of interceptors, refer to the gRPC guide: https://grpc.io/docs/guides/interceptors lthough this functionality logically belongs in the `drpcserver` package, implementing it within `drpcmux` offers a more practical approach, avoiding a substantial refactor. create `drpc.ClientConn` wrapping a `drpc.Conn` drpcclient: create a clientconn wrapping drpc.Conn Here, we are updating constructor of clientConn to take in a `drpc.Conn` type object instead of a supplier of `drpc.Conn`. This simplifies the usage/construction of clientConn, and moves the responsibility of invoking a supplier/dialer to the caller of constructor. This way, closing of drpcpool would be a clear duty of the caller of this constructor. codegen: create RPC service interface with gRPC & DRPC adapters (#4) Define a shared `RPC<Service>Client` interface to wrap both gRPC's `<Service>Client` and DRPC's `DRPC<Service>Client`. Streaming methods will also return the common `RPC<Service>_<Method>Client`. Adds two simple adapters: - `NewGRPC<Service>ClientAdapter` - `NewDRPC<Service>ClientAdapter` They implement `RPC<Service>Client`, so we can swap clients at runtime via the cluster setting. codegen: generate common interfaces for stream server and client (#2) These interfaces contain the shared methods between DRPC and gRPC stream servers and clients as used in cockroach. codegen: update `RecvMsg` for gRPC compatibility (#3) It's already here in the implementation of the StreamServer implementations but not available through interface, so essentially it's hidden. In this change we update its parameters to become compatible with gRPC and added that definition in the interface. drpcclient: add client interceptor support in drpc (#6) Interceptors are not natively supported by drpc. Here, we add support for interceptors in drpc. The changes include definition of client interceptor interface types, logic to chain multiple unary/stream interceptors. This change also introduces a new struct `drpcclient.ClientConn` which satisfies the drpc.Conn interface, and decorates underlying drpc Connection object with interceptors. The struct drpcclient.ClientConn is defined such that it can be initialized with a dialer function which supplies underlying connection either concretely or from a pool. Functional options pattern is employed to provide flexibility with addition of unary and chain interceptors on ClientConn struct. Epic: CRDB-50378 Fixes: #146086 codegen: use cockroachdb/errors package The `cockroachdb` linters don't allow using the standard `errors` package, even in generated `*_drpc.pb.go` files. This change replaces its usage in `UnimplementedServer` with `cockroachdb/errors` to comply with linting requirements. drpcstream: runtime/trace support Change-Id: I91ebe74d3058a90ef8cc327accc9ac307fb323ab protoc-gen-go-drpc: generate GetStream method for client streams Change-Id: I0c87f4e8865e7eb153d28f045e402bbe5afbef8b drpcpool: fix bug with unset context field Change-Id: I2a58043a6901e21dda44afff2eae031a58e6b60b readme: update grpc version and benchmark results Change-Id: I6f26f75806e66d357a0c9faf977a335c98912401 drpcconn: expose read/written byte stats Change-Id: I852f8bebd20a4ff41bda112abf684f065e9154e1 drpcstats: add stats for bytes read/written by rpc Change-Id: I449a3447d9b320aef0d1296e46d083ffb22130b4 drpcwire: handle bytes even with an error from Reader adds a helper method that ensures the result from the read call is either valid data + nil error or no data + an error. if the reader spins returning no data and no error long enough, the helper returns a no progress error. Change-Id: If80f279f0597362f37de5182c99d3216683f62b6 all: some version bumps also removes the check-errs lint because it wasn't providing much value and was starting to become flaky with weird compiler like errors (missing symbol resolution, etc). a quick look at the source code of the package showed that it was not much more than a wrapper around go analysis tools so it's unclear what the problem is. Change-Id: I34056cdca57b39aef41b2b311906e48cbd96afcd drpcwire: add details to errors This additional information can help diagnose what might be going wrong. Change-Id: I7a1f4f2d8e7a50438874e170477ba418ebe9e296 README: update domain for go.elara.ws/drpc go.mod: bump language version to 1.19 Change-Id: I7996a08193efafcddda6bb1874f93dc085af9e88 drpcstream: SendCancel is busy when writing at all SendCancel would return that it was not busy if the stream was terminated, but there are ways for the stream to be terminated while it is still writing. Instead, check if any writes are happening before the check for termination. Change-Id: Ib3424d351f66741961cb7fe32795c0c0dfa99db2 all: bump flake and regen Change-Id: I44b395121aaad72c8ef6f8e5d3d7891e8f5a7518 drpcpool: make generic Change-Id: I1c643adfa0b5d4adb8f6a477697d869015d6da19 drpc{manager,stream}: allow passing error to SendCancel this way the stream will return the error from the context that was canceled instead of always context.Canceled. Change-Id: I970ef27d492a0918894af4ab6d3d292891ebb069 drpcmanager: check syscall pkg for reset error message Change-Id: I31dbf6aaf5233ea7a7da891b27e2c6fc080a04fd drpcmanager: catch more connection resets Change-Id: I1895723143cc727df7879c241510b89cfe743779 drpcmanager: treat connection reset as a type of ClosedError Change-Id: Icc49f3104f373de2fcf0b94ef060c0d2ea5d3ca5 drpcstream: SetManualFlush SetManualFlush is a new feature that allows for clients who want to inject messages into the write buffer without flushing right away. Example use case: flusher := stream.(interface{ GetStream() drpc.Stream }).GetStream().(interface{ SetManualFlush(bool) }) flusher.SetManualFlush(true) err = stream.Send(&pb.Message{Request: "hello, "}) flusher.SetManualFlush(false) if err != nil { return err } // the next send will send both messages in the same write // to the underlying connection. err = stream.Send(&pb.Message{Request: "world!"}) if err != nil { return err } Change-Id: I32928495e96e6f7504cc0be288a66811b637d698 drpc{stream,manager}: stop managing stream on finished Change-Id: I75f813ea02bded14fcc740a55be351656c402861 drpcstream: fix data race in packet buffer handling the packet buffer would go into a finished state when closed even if someone is still holding and using the data slice from get, allowing put to unblock and causing a data race. this adds a held boolean to the packet buffer state so that close will wait for any handing of the data buffer before going into the finished state. fixes #48 Change-Id: I131fc7addb26153e31b68015e58a2e400c1ce8f5 all: tiny fixes and corking fix Change-Id: I545415aca93dc443e3d071e64f95e1b722bfd1f7 flake.nix: update staticcheck Change-Id: Ie3e0898bf6157e761bc19cab8192736c54bcfba0 drpcserver/util.go: remove unused nolint directive Change-Id: Ifa62bbac9ac230ce9d28a2f561c61c8dd2738731 go.mod: bump lang to 1.18 Change-Id: I312b8a79c23f2fbdbe9f92ce417730bdceb17e5e drpcpool: add debug logging and small poolConn optimization Change-Id: Ie2c4b300fa0f88f6647e838742f7bf171753e851 drpcmanager: drop buffers after runs of small packets this avoids large heap usage when a single packet is large. after enough "small" packets have been sent where small is defined as less than 1/4th of the capacity of the buffer, then we drop the buffer for the next read. Change-Id: Ibc55d267764daff9e8fb9bb2c1e9630dbc7d6f74 drpcpool: use linked list for eviction the typical answer for linked lists is "dont", but a benchmark here shows a 10x reduction in cpu usage when there's a large number of connections (~10000) in the pool. Change-Id: Id3a3e8682e743ad553b91fc339218901750124dd drpc{conn,manager,stream,pool}: soft cancel support this commit adds "soft cancel" support where if a stream is canceled, it attempts to send a small cancel message instead of hard terminating the transport. the manager will block creation of new streams until the stream has sent this soft cancel message successfully, and terminate if it can't send it right away. the manager exposes a channel that is used by the pool to check if it is still in a state where it is soft canceling so that the pool can skip over connections in that state. one thing to consider is that with connection pooling, a soft cancel on a connection that has a bunch of data in the send queue could still proceed and consider the connection "unblocked" even though there will be some head of line blocking for the next rpc. perhaps some future code can either look at the send queue directly or wait for acknowledgement from the remote side. the latter option is not backwards compatible, though, so some care would have to be taken to figure out how to make that work. this adds more tests to the pool and adds some fixes found because of those tests. almost all of the tests from the storj rpcpool cache have been ported except for the fuzz test which is difficult to port due to this design having the cache and pool combined. Change-Id: I04a00b91e6a40fb3f1144a9a4f85bd2a9867d5f4 integration: add test around pooled cancel Change-Id: If8160587ed2a42d54483798a3a7dbd3c8528610d drpc{conn,stream}: skip flush until first read this allows 0-rtt rpcs to be made Change-Id: I14925261df11c0fe763478aa300d0cabd46ee280 README: add concurrent rpc 3rd party package Change-Id: Id0c4359a735c4cce717b164fa686d0af5eee403c drpcwire: regenerate README Change-Id: Ia0e346b9562528adddfb3af84c871f9aaf0c15ac README: Replace "bindings" by "support" (#47) The README used the word "bindings" for referring DRCP implementation in other programming language. Because other languages have to implement DRPC entirely without relying on the Go implementation. Fix links to examples (#45) Co-authored-by: Jeff Wendling <jeff@storj.io> drpctest: add new package for testing focused stuff Change-Id: I0e67886e35e0ab59e4ad21a7c4a8fe26efa6b0da drpc{wire,stream}: pass through Control frames rather than just drop frames with the control bit set we can assemble them into packets like usual with the packet control bit set. upper layers are then responsible for ignoring any unknown control packets for forwards compatibility. Change-Id: Icaf43d4f72c158f740701071e2c5b7ba4215a1ba all: bump flake/go versions and regenerate Change-Id: I9a084aec7eff9fef365775b2511fcc77a427be14 drpcwire: add back KindCancel for soft cancel support Change-Id: I285f9bd31b7fb1d3d20b7f4421e822d482afa421 drpcpool: add simple connection pool this is a pool designed around the techniques used in the connection pool in storj.io/common/rpc/rpcpool, but somewhat simplified and generic in the key type. it adds about 10% lines of code to the package, but this is a common issue multiple users have hit and either given up or invented themselves. Change-Id: Ie86f523fb36283ac63b5757f6652a39971b1aca2 grpccompat: bump libraries, apply fixes, update README this reruns the benchmarks, updates the table and lines of code counters at the bottom, and adds a blurb about why lines of code is important. Change-Id: Ia2f5fefb2c7cd7eebb7ad2f0b3d264855ef8fb3b drpcserver: fix lint issue Change-Id: I6b9c38e7f4634f82869925e28f7ecc0f981c6e63 examples/opentelemetry: add example with opentelemetry tracing Change-Id: I5ad8fffd922fa7f43bbd94e7e19a187c98034c29 all: replace deprecated ioutil Change-Id: Id5b4c9549aee21ba3742ef246c7fe84a923f0013 all: fix formatting Change-Id: I06fb50078d8369d09f11d0e63c87b058d05fe590 drpcmanager: reduce memory allocation in Reader (#40) Also, a `go fmt` while I'm here. examples: fix some small bugs - route before calling Run instead of concurrently - fix the http client parsing of json response Fixes #28 Change-Id: Iafe341cf67984abedd440eb1f13b72c12f081a97 drpcmigrate: avoid potential double close of default listener Change-Id: I700a43f306b000e1540558c0f302ca751b38ea05 drpcmigrate: close default listener properly Closing dprcmux can hang forever, if default Listener is used (.Accept). It should be closed similat to all other routers. Change-Id: I51dd56f5c319d18b09df5db218d14b7dd2b93697 drpcmanager: expose error from transport failures A cancel can sometimes only be performed by closing the underlying transport because that is the only way to interrupt a read/write syscall. But, rather than assume that every error from the transport failing is due to a cancel, only assume that an io.EOF means that. This can help with debugging when the remote side dies due things besides a "clean" close of the underlying transport. Closes #31 Change-Id: Ia655d07984f6abe0492e7f9300d2638a1c82b4d4 drpcwire: limit frame size based on packet size if packets are huge, then frames should be allowed to be too. similarly, if packets are small, then there is no reason for huge frames. Change-Id: I36b46abefbd7c49bfa807c62e00c739015fc3703 regen docs Change-Id: I04ac7c0c5bea05a95ee38cd0584ca624ce60f077 drpcwire: expose buffer size on Reader (#30) The maximum was hardcoded at 4MB. This allows it to be configurable for larger payloads. An option is exposed in `dprcmanager.Options`. drpcmanager/drpcstream: label streams with cli/srv kinds Change-Id: Ie324f958cadfa038d4107ea1459dc38a45c5a82f nix: add missing tools to flake.nix these were missed due to a non-hermetic environment being used to create it in the first place. Change-Id: I3e4db14f525890be6e067a2873dacb0a02e6ff1d drpchttp: fix linting issue Change-Id: I0d01dc856ba7bcd81229ebd9659e42b49535a657 drpcstream: return ClosedError for remote closes In logging it's helpful to distinguish between an actual stream error and a remote closing, which is to be expected. Updates https://github.com/storj/storj/issues/4609 Change-Id: I5e70413484820efb9d0d2c8792c9713de9f8658f drpc: add comment on interface Transport (#20) Add comment that drpc.Transport is compatible with net.conn drpcstream: set cancel state more often if a Close has started, and we Cancel, we should still set the cancelled state while Close is still executing. this is so that we don't surface the underying transport being closed errors. Change-Id: I2f3924f7b03c3952ab58b9952e4ae18ef1a5a88f drpcserver: log callback and temporary error sleeps It was possible before to spin a cpu core trying to accept from the tcp listener when it always returned a temporary error. Now, it will sleep for 500ms before trying again. Change-Id: I59d1503b4d14e9ded78d78c608ea4c416a4e10df drpcstream: control for max buffer size with many concurrent and potentially cached streams that send large messages, dropping large buffers can help with peak memory usage. Change-Id: Ia99f30ff78307bea909fde813c2ecb6667a4b1d8 Fixes "File is not `gofmt`-ed with `-s` (gofmt)" (#22) cmd/protoc-gen-go-drpc: proto3 optional support this additionally bumps the google.golang.org/protobuf dependency to v1.27.1, and the nix flake dependencies for protoc from v3.16.0 to v3.18.0. adds an optional field into the grpccompat tests rather than the integration tests because we generate using gogoprotobuf sometimes in the integration tests which does not support optional fields. Change-Id: Idb52705b2e57588120a7f6af7bc2db36ddde3d09 mod: bump semantic version to go 1.17 This enables module graph pruning and lazy module loading. Change-Id: I5a942d6f9bfedaa3ee682f08216cef04b7e5b633 drcpserver: fix windows build Change-Id: Ia4d2d138ece4a5b6d736e5e72405960191ac9ff1 drpcmanager: fix closing race condition in some situations, newStream could be blocked forever trying to send a stream into m.streams when the manageStreams goroutine has already exited due to manager terminating. this changes the send into a select that can be canceled by the manager terminating. also, there was a race in the tests for the muxer where rare failures could happen if the Route call was not scheduled before the connection with the given prefix was handled. Change-Id: If2033d8b8e3b2c3e3a50e8a72337bc263458321e drpcserver: add checks for temporary network errors when a temporary network error occurs, it should not crash the entire process. This PR also adds a method to check for specific windows errors that should be considered temporary. Change-Id: I258bb36103c028754c6829ec8618388cc1db4524 drpcmanager: remove allocations introduced in go1.17 launching a goroutine ends up allocating a closure object in go1.17 that it didn't before. this maybe has something to do with the register abi? in any case, because we only ever have 1 stream executing at a time, we can have a single goroutine that just runs the stream and pass it the information needed and launch that single goroutine at the start. this doesn't have an effect on any benchmarks, except for go1.17 benchmarks. Change-Id: Ifadce4a73423c7e7c31c296a2f22fd7c11b674b2 drpchttp: some renaming/comments Change-Id: I36e266178700edde3ffb3480d3efbc913cc7dcd2 drpchttp: add grpc-web support This refactors drpchttp to add expansion by defining the Protocol interface, and adding options for external consumers to define their own protocols. Additionally, it reverts the un-tagged option to control how Twirp codes are processed and adds reflect based code to grab the code from errors without having to import the Twirp module. It also reverts the breaking change introduced with the Twirp change to the New function by adding a NewWithOptions function much like how the drpc{conn,stream,manager,server} packages work. Change-Id: Ibaae7fa18d78640a6d2ee85d75826afc24f37803 ensure twirp compatibility Change-Id: I12299f3e4f130531271b577d9cbec0a65bda531d make tidy Change-Id: I281279c740e0fd7ce87e6fb1bb1405e5df971091 Add external packages, other languages, etc. Change-Id: Ieeb51296bcd54fa28967f5adbe23ed863467a6ba drpc: add nix flake support this allows two things: 1. running `nix shell github:storj/drpc` puts you into a shell that has `protoc-gen-go-drpc` installed, which is neat. 2. in the directory, running `nix develop` puts you into a shell that has all of the binaries required to run all the lints/generates/ci/etc. in fact, you can do `nix develop -c make` to run it all which is neat. both of those commands are fully hermetic and repoducible so they will always work forever on any system which is neat. no idea if i'm going to ever merge this, but nix flakes are cool. Change-Id: Ia2f1c53632da80b7cb2bfa832420aeeccc5f851c drpcwire: disallow repeating ids and more tests Change-Id: Ib53b66bb6fccd35476cc4371d931ad751b1b8b6d drpccache: add unit tests Change-Id: I819decfad4effc2737d5a01a7b7959b0ca0cbf99 drpc: remove Transport method from Conn Pooled connections or wrappers that dispatch to different underlying Conns will not have a canonical "transport" that they are writing to. The concrete drpcconn.Conn type can still export the method, but don't require it for all implementers. Change-Id: I8ee229a446bd9e0042fb4bf759c5ca2aa247b604 README: add loc estimate to keep me honest it helps to keep track of where most of the complexity is and makes the "few thousand lines of code" quantified. Change-Id: Iaa53ecc9a6656482bc49f700e963edf58946460e drpcmigrate: remove test dependency on x/sync Change-Id: I5240607abf0710ff42c64bb4ceae5cc41f684ede drpcwire: remove bufio.Scanner use it's a bit more code but there's some fairly big performance wins by managing our own buffers. for example, we can read into a large slice and remove frames until there are no more and only then move the tail of the slice to the front. that reduces copying from potentially quadratic to linear. name old/op new/op delta Unitary/Small 8.72µs 8.59µs -1.52% Unitary/Med 11.2µs 11.1µs -1.22% Unitary/Large 633µs 633µs ~ InputStream/Small 834ns 759ns -9.00% InputStream/Med 2.42µs 2.00µs -17.27% InputStream/Large 259µs 249µs -4.09% OutputStream/Small 846ns 757ns -10.48% OutputStream/Med 2.38µs 1.99µs -16.13% OutputStream/Large 241µs 239µs ~ BidirStream/Small 3.35µs 3.30µs -1.62% BidirStream/Med 5.01µs 4.94µs -1.38% BidirStream/Large 551µs 546µs ~ Change-Id: Ia21ec5097907b97c6059aac483927e1bc2f53336 lint: match github.com/storj/ci linting rules Change-Id: Ie776659984a2fca20548928e20bb3eed46714635 drpcdebug: more logging with more information Change-Id: Icde03daa1266f4cef8b045857dbb1fbe2942b957 drpcstream: remove three allocations this removes an allocation for a context to return the transport by passing it into the stream itself and having the owned stream context value return it directly. this removes another allocation by not having the manager look at the Terminated signal. it does this by passing in a channel that the stream sends once on. the manager ensures that only one stream is active at a time, so this is sufficient. both of the above were accomplished by extending the stream options with a set of internal options that can only be modified and read with a new internal drpcopts package. this also removes an allocation by avoiding callbacks by avoiding drpcwire.SplitN when writing. the compiler was unable to prove that the passed in data slice did not escape, so the string to []byte conversion when passing in the rpc name had to allocate. with the more inlined code, the compiler is now able to prove it does not escape and avoids making a copy. Change-Id: Id038f94eaeab32f31e6f88cbc06a502da63c9b05 fuzz: add some gofuzzbeta tests they don't do anything for now, but one day they will. Change-Id: Ie212126d65ed992a9662a948206d9a438e36da42 drpc{manager/stream}: add some randomized tests this also fixes a bug where the drpcstream packet buffer could panic under concurrent message send and cancellation. it also changes the mamnager to close any existing stream when a packet with a later id is received. additionally, benchmarks improve. this is prolly due ditching a channel for the packet buffer and reducing the memory allocations that it does. name old/op new/op delta Unitary/Small 9.71µs 9.36µs -3.68% Unitary/Med 12.3µs 12.0µs -2.47% Unitary/Large 643µs 593µs -7.75% InputStream/Small 835ns 809ns -3.16% InputStream/Med 2.58µs 2.44µs -5.53% InputStream/Large 259µs 247µs -4.72% OutputStream/Small 849ns 814ns -4.12% OutputStream/Med 2.53µs 2.43µs -3.96% OutputStream/Large 245µs 242µs -1.07% BidirStream/Small 3.68µs 3.43µs -6.97% BidirStream/Med 5.38µs 5.04µs -6.36% BidirStream/Large 559µs 555µs -0.69% name old allocs/op new allocs/op delta Unitary/Small 14.0 12.0 -14.29% Unitary/Med 16.0 14.0 -12.50% Unitary/Large 16.0 14.0 -12.50% Change-Id: I8a778a546201d0f0310d152816a1233a2d687a49 drpcerr: add Unimplemented code Change-Id: Idb800efbb31ebef7a3aa25f06da6149a7820a2dd add test that error messages are sent Change-Id: Iccdcdd2d3117aa777e76a0f5a568e961a16ef8d9 drpc{conn,manager,stream}: refactor and perf improvements this changes the manager to more consistently be reading from the underlying transport. it does this by always delivering messages to the active (or most recently active) stream as well as discarding any messages for older streams. in order to do this, it has to keep track of the active stream better. a secondary benefit of that is we don't have to send stuff over channels as often, which is a performance win. this adds a "ManualFlush" stream option that causes the stream flushing semantics to more closely match gRPC: it will only flush writes when the buffer is too large, a receive is called and there is data in the buffer, or when manually asked. this accounts for most of the performance difference on the stream benchmarks for small and medium sizes, and makes all of the benchmarks either as good as or better than grpc. the following is the performance delta for DRPC against itself with this change applied. note that for the speeds in particular, the measurements aren't very valid because some changes were made to the benchmarks themselves that changed the number of bytes being sent. tbh this change got away from me a bit. it should have been like 8 smaller changes. oh well. name old time/op new time/op delta Unitary/Small 15.6µs ± 2% 9.7µs ± 1% -37.56% Unitary/Med 19.7µs ± 2% 12.3µs ± 1% -37.58% Unitary/Large 654µs ± 4% 643µs ± 6% ~ InputStream/Small 2.59µs ± 1% 0.84µs ± 1% -67.72% InputStream/Med 3.59µs ± 5% 2.58µs ± 1% -27.98% InputStream/Large 256µs ± 4% 259µs ± 1% +1.43% OutputStream/Small 2.68µs ± 1% 0.85µs ± 2% -68.29% OutputStream/Med 3.60µs ± 2% 2.53µs ± 1% -29.69% OutputStream/Large 248µs ± 1% 245µs ± 1% -1.14% BidirStream/Small 5.50µs ± 2% 3.68µs ± 0% -33.02% BidirStream/Med 7.52µs ± 5% 5.38µs ± 1% -28.53% BidirStream/Large 586µs ± 7% 559µs ± 1% -4.56% name old speed new speed delta Unitary/Small 130kB/s ± 0% 208kB/s ± 4% +59.62% Unitary/Med 104MB/s ± 2% 167MB/s ± 1% +60.19% Unitary/Large 1.61GB/s ± 3% 1.63GB/s ± 5% ~ InputStream/Small 774kB/s ± 1% 2396kB/s ± 1% +209.41% InputStream/Med 573MB/s ± 5% 795MB/s ± 1% +38.73% InputStream/Large 4.10GB/s ± 4% 4.04GB/s ± 1% -1.44% OutputStream/Small 1.49MB/s ± 3% 2.36MB/s ± 2% +58.22% OutputStream/Med 572MB/s ± 2% 812MB/s ± 1% +42.07% OutputStream/Large 4.23GB/s ± 1% 4.28GB/s ± 1% +1.15% BidirStream/Small 363kB/s ± 2% 540kB/s ± 0% +48.82% BidirStream/Med 273MB/s ± 5% 380MB/s ± 3% +39.35% BidirStream/Large 1.79GB/s ± 6% 1.87GB/s ± 1% +4.65% name old alloc/op new alloc/op delta Unitary/Small 2.15kB ± 0% 1.54kB ± 0% -28.34% Unitary/Med 8.55kB ± 0% 7.94kB ± 0% -7.17% Unitary/Large 3.23MB ± 0% 3.16MB ± 0% -2.01% InputStream/Small 80.0B ± 0% 80.0B ± 0% ~ InputStream/Med 2.13kB ± 0% 2.13kB ± 0% ~ InputStream/Large 1.08MB ± 0% 1.05MB ± 0% -2.89% OutputStream/Small 160B ± 0% 80B ± 0% -50.00% OutputStream/Med 2.21kB ± 0% 2.13kB ± 0% -3.62% OutputStream/Large 1.06MB ± 0% 1.05MB ± 0% -1.02% BidirStream/Small 240B ± 0% 240B ± 0…
1 parent 0ae65f3 commit 6fc345c

1 file changed

Lines changed: 157 additions & 0 deletions

File tree

drpcmanager/flow_control_test.go

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
// Copyright (C) 2026 Cockroach Labs.
2+
// See LICENSE for copying information.
3+
4+
package drpcmanager
5+
6+
import (
7+
"context"
8+
"errors"
9+
"io"
10+
"net"
11+
"sync/atomic"
12+
"testing"
13+
14+
"github.com/zeebo/assert"
15+
16+
"storj.io/drpc"
17+
"storj.io/drpc/drpcstream"
18+
"storj.io/drpc/drpctest"
19+
"storj.io/drpc/drpcwire"
20+
"storj.io/drpc/internal/drpcopts"
21+
)
22+
23+
// fcManagerOptions returns manager Options whose streams have flow control
24+
// enabled via the internal stream option.
25+
func fcManagerOptions(window, highWater, threshold int64) Options {
26+
stream := drpcstream.Options{SplitSize: 64 << 10}
27+
drpcopts.SetStreamFlowControl(&stream.Internal, drpcopts.FlowControl{
28+
Enabled: true,
29+
StreamWindow: window,
30+
HighWater: highWater,
31+
GrantThreshold: threshold,
32+
})
33+
return Options{Stream: stream}
34+
}
35+
36+
// Enabling flow control through the manager's Stream options installs windows
37+
// on every stream it creates (client and server), so a message larger than the
38+
// send window only completes if credit grants flow across the real connection.
39+
func TestManager_FlowControlEndToEnd(t *testing.T) {
40+
ctx := drpctest.NewTracker(t)
41+
defer ctx.Close()
42+
43+
cconn, sconn := net.Pipe()
44+
defer func() { _ = cconn.Close() }()
45+
defer func() { _ = sconn.Close() }()
46+
47+
// 128 KiB window = 2 frames of initial credit.
48+
opts := fcManagerOptions(128<<10, 1<<20, 64<<10)
49+
50+
cman := NewWithOptions(cconn, Client, opts)
51+
defer func() { _ = cman.Close() }()
52+
sman := NewWithOptions(sconn, Server, opts)
53+
defer func() { _ = sman.Close() }()
54+
55+
// 256 KiB is four frames: two fit in the initial window, the rest can only
56+
// be sent as the server dispatches frames and returns credit.
57+
msg := make([]byte, 256<<10)
58+
59+
ctx.Run(func(ctx context.Context) {
60+
stream, err := cman.NewClientStream(ctx, "rpc", drpc.CompressionNone)
61+
assert.NoError(t, err)
62+
assert.NoError(t, stream.RawWrite(drpcwire.KindInvoke, []byte("rpc")))
63+
assert.NoError(t, stream.RawWrite(drpcwire.KindMessage, msg))
64+
assert.NoError(t, stream.Close())
65+
})
66+
67+
ctx.Run(func(ctx context.Context) {
68+
stream, _, err := sman.NewServerStream(ctx)
69+
assert.NoError(t, err)
70+
defer func() { _ = stream.Close() }()
71+
72+
got, err := stream.RawRecv()
73+
assert.NoError(t, err)
74+
assert.Equal(t, len(got), len(msg))
75+
76+
_, err = stream.RawRecv()
77+
assert.That(t, errors.Is(err, io.EOF))
78+
})
79+
80+
ctx.Wait()
81+
}
82+
83+
// sniffConn tees every byte read off the connection into w, where a frame
84+
// parser classifies it.
85+
type sniffConn struct {
86+
net.Conn
87+
w io.Writer
88+
}
89+
90+
func (s sniffConn) Read(p []byte) (int, error) {
91+
n, err := s.Conn.Read(p)
92+
if n > 0 {
93+
_, _ = s.w.Write(p[:n])
94+
}
95+
return n, err
96+
}
97+
98+
// Tripwire for accidental enablement: a default-configuration connection must
99+
// never put a KindWindowUpdate on the wire. Both directions are sniffed; the
100+
// KindMessage assertion proves the sniffer actually parsed the traffic.
101+
func TestManager_DefaultConfigEmitsNoWindowUpdates(t *testing.T) {
102+
tr := drpctest.NewTracker(t)
103+
defer tr.Close()
104+
105+
var sawMessage, sawWindowUpdate atomic.Bool
106+
sniff := func(c net.Conn) net.Conn {
107+
pr, pw := io.Pipe()
108+
go func() {
109+
rd := drpcwire.NewReader(pr)
110+
for {
111+
fr, err := rd.ReadFrame()
112+
if err != nil {
113+
_, _ = io.Copy(io.Discard, pr) // keep the tee unblocked
114+
return
115+
}
116+
switch fr.Kind {
117+
case drpcwire.KindMessage:
118+
sawMessage.Store(true)
119+
case drpcwire.KindWindowUpdate:
120+
sawWindowUpdate.Store(true)
121+
}
122+
}
123+
}()
124+
t.Cleanup(func() { _ = pw.Close(); _ = pr.Close() })
125+
return sniffConn{Conn: c, w: pw}
126+
}
127+
128+
cconn, sconn := net.Pipe()
129+
defer func() { _ = cconn.Close() }()
130+
defer func() { _ = sconn.Close() }()
131+
132+
cman := NewWithOptions(sniff(cconn), Client, Options{})
133+
defer func() { _ = cman.Close() }()
134+
sman := NewWithOptions(sniff(sconn), Server, Options{})
135+
defer func() { _ = sman.Close() }()
136+
137+
tr.Run(func(ctx context.Context) {
138+
stream, _, err := sman.NewServerStream(ctx)
139+
assert.NoError(t, err)
140+
defer func() { _ = stream.Close() }()
141+
got, err := stream.RawRecv()
142+
assert.NoError(t, err)
143+
assert.Equal(t, len(got), 256<<10)
144+
_, err = stream.RawRecv()
145+
assert.That(t, errors.Is(err, io.EOF))
146+
})
147+
148+
stream, err := cman.NewClientStream(context.Background(), "rpc", drpc.CompressionNone)
149+
assert.NoError(t, err)
150+
assert.NoError(t, stream.RawWrite(drpcwire.KindInvoke, []byte("rpc")))
151+
assert.NoError(t, stream.RawWrite(drpcwire.KindMessage, make([]byte, 256<<10)))
152+
assert.NoError(t, stream.Close())
153+
tr.Wait()
154+
155+
assert.That(t, sawMessage.Load())
156+
assert.That(t, !sawWindowUpdate.Load())
157+
}

0 commit comments

Comments
 (0)