Repository navigation
Named Pipe connection behavior matching Unix Stream behavior - #345
Conversation
A client built while the agent's named pipe didn't exist failed to build, and a client whose pipe broke, for example after an agent restart, failed every write until the application restarted. UDP and Unix sockets don't fail this way. NamedPipeClientChannel now opens the pipe on the first write. When a write fails, it closes the pipe, and the next write opens it again. This matches UnixStreamClientChannel and datadog-go's Windows pipe writer. A missing pipe is now reported to the error handler for each dropped write instead of failing build(). SVLS-8246
CloseHandle on a synchronous pipe can wait for the listening thread's pending ReadFile. When a client is still connected and idle, as in reconnects_after_server_restart, close() could then block until the client sends again. close() now only marks the server closed, and the listening thread closes the handle after its current read returns.
Lewis-E
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64cc96b513
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (fileChannel == null) { | ||
| randomAccessFile = new RandomAccessFile(pipe, "rw"); |
There was a problem hiding this comment.
close() calls disconnect(), which only nulls these fields, so the next write() always enters this branch and silently reopens the pipe. That violates the WritableByteChannel invariant that closure is permanent (and isOpen() also reports false before the first write), so a late internal write—such as one racing a nonblocking shutdown—can recreate the native handle after the client has closed it. Track an explicit closed flag and throw ClosedChannelException after close while reserving null for reconnectable disconnects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ close() now marks the channel closed, so a later write() throws ClosedChannelException without opening the pipe.
@vickenty UnixStreamClientChannel has the same gap: its close() only calls disconnect(), so a later write() reconnects, and its isOpen() throws a NullPointerException before the first connect. Want to include that change in this PR while we're updating things?
There was a problem hiding this comment.
No, let's leep this PR scoped to windows pipes only.
close() only disconnected, so a later write reopened the pipe. A late write during a non-blocking stop() could reopen it after the client closed the channel. close() now marks the channel closed: writes throw ClosedChannelException without opening the pipe, and isOpen() reports false only after close(). A failed write still disconnects, and the next write reopens the pipe.
A JVM agent can set system properties after startup but not environment variables. serverless-compat-java will set dd.dogstatsd.pipe.name to a per-function pipe on Azure Functions shared plans, so a client built with default settings now uses it. Precedence: explicit builder settings, then the system property, then the existing DD_DOGSTATSD_URL, DD_DOGSTATSD_PIPE_NAME and DD_AGENT_HOST environment variables. The property pipe uses the same channel as every other named pipe. Merge after DataDog#345, which opens that channel on the first write, so a client built before its pipe exists doesn't fail build(). SVLS-8246
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
dbccb10
into
DataDog:master
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
A JVM agent can set system properties after startup but not environment variables. serverless-compat-java will set dd.dogstatsd.pipe.name to a per-function pipe on Azure Functions shared plans, so a client built with default settings now uses it. Precedence: explicit builder settings, then the system property, then the existing DD_DOGSTATSD_URL, DD_DOGSTATSD_PIPE_NAME and DD_AGENT_HOST environment variables. The property pipe uses the same channel as every other named pipe. Since DataDog#345 that channel opens on the first write, so a client built before its pipe exists doesn't fail build(). SVLS-8246
Named pipes now connect the way Unix sockets do. Before, a missing pipe made
build()fail withStatsDClientException: Failed to start StatsD client, and a pipe that broke later stayed broken. Now a missing pipe doesn't failbuild(), and the client reopens the pipe after the server restarts. @vickenty suggested this in #344, which will rebase on this PR.Implementation
NamedPipeClientChannelstores the pipe name and opens the pipe on the first write. When a write fails, it closes the pipe. The next write opens the pipe and tries again. Afterclose(), the channel stays closed: writes throwClosedChannelExceptionwithout opening the pipe. This matches the behavior ofUnixStreamClientChannelin this repo and the Windows pipe writer in datadog-go, which defers the connection to the first write and drops it after a write error. It also fits the DogStatsD Client API Standards lifecycle rule that constructors return clients ready to use.Blast radius and default-path deltas
Affects clients using a named pipe address:
namedPipe(...)DD_DOGSTATSD_PIPE_NAMEDD_DOGSTATSD_URLthat starts with\\.\pipe\UDP and Unix socket clients don't run this code.
build()no longer throws. Each write while it's missing is dropped, counted in client telemetry, and passed to the error handler as aFileNotFoundException. This is a behavior change for any customers expecting a startup failure. Clients without an error handler use the default no-op handler. Their telemetry goes through the same pipe by default, so a wrong pipe name is silent unless telemetry has its own address.namedPipe(...)and retriesbuild()every 10 seconds when it throws. That retry will no longer trigger from a named pipe issue; the channel reconnects instead. The tracer's error handler logs dropped writes through a rate-limited logger (once per 5 minutes).Performance: with the pipe open, each write adds a null check and an uncontended lock. With the pipe missing, each dropped packet adds one failed open attempt, which doesn't wait, on top of the exception and error-handler call that a broken pipe already causes today. This matches
UnixStreamClientChanneland datadog-go. Not benchmarked.Tests
NonBlockingStatsDClientBuilderTest.missing_named_pipe_does_not_fail_build(all platforms) checks that a missing pipe builds and reports aFileNotFoundExceptionon write. It fails on master.NamedPipeTest.sends_to_pipe_created_after_build(Windows) checks that a pipe created afterbuild()receives the next metric.NamedPipeTest.reconnects_after_server_restart(Windows) closes the server, waits for the client to see the broken pipe, starts a new server on the same name, and checks that a later metric arrives.NamedPipeClientChannelTest.write_after_close_throws_without_opening_the_pipe(all platforms) checks that a closed channel doesn't reopen the pipe.NamedPipeDummyStatsDServer.close()now only marks the server closed, and the server's listening thread closes the pipe after its current read returns. Closing a synchronous pipe from the test thread can wait for that read, which hung the restart test while the client was idle.The Windows tests run only in the
windows-latestCI job.See also: this comment, SVLS-8246, SVLS-9467.