[EventPipe] End user_events session upon continuation_stream closure #117435

mdh1418 · 2025-07-08T20:28:15Z

User_events-based EventPipe sessions should end when the profiling process disconnects from the Diagnostic Port.

This PR does the following:

Adds the poll_func to IpcStream vtables
Adds ds_ipc_stream_poll based on ds_ipc_poll
Pass the continuation stream to user_events sessions
Extends the eventpipe streaming thread for user_events with ep_ipc_continuation_stream_connection_closed to detect disconnects.

Testing

Manual E2E testing with the same C program as #115265, except putting a getchar before closing the socket from the client side.

mdh1418 · 2025-07-08T20:28:54Z

cc: @pavelsavara for websocket poll implementation

Copilot

Pull Request Overview

This PR adds support for detecting remote disconnections on user_events-based EventPipe sessions by extending IPC streams with polling capabilities and wiring up a continuation stream to session logic.

Introduce IpcStreamPollFunc in IPC vtables and implement ds_ipc_stream_poll variants
Define IpcContinuationStream and add ep_ipc_continuation_stream_connection_closed for hangup detection
Refactor session streaming thread to handle EP_SESSION_TYPE_USEREVENTS and unified ep_session_type_uses_streaming_thread predicate

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/native/eventpipe/ep-types-forward.h	Forward-declare `IpcContinuationStream`
src/native/eventpipe/ep-stream.h	Define `IpcContinuationStream` struct and closure API
src/native/eventpipe/ep-stream.c	Add `ep_ipc_stream_poll_vcall` and continuation-stream check
src/native/eventpipe/ep-session.h	Add `continuation_stream` field and streaming-thread predicate
src/native/eventpipe/ep-session.c	Refactor streaming thread to support user events and cleanup
src/native/eventpipe/ep-ipc-stream.h	Add `IpcStreamPollFunc` and `ep_ipc_stream_poll_vcall` prototype
src/native/eventpipe/ep-ipc-pal-types.h	Define `EventPipeIpcPollEvents` enum and poll timeout constant
src/native/eventpipe/ds-ipc.c	Replace `DS_IPC_POLL_EVENTS_` with `EP_IPC_POLL_EVENTS_`
src/native/eventpipe/ds-ipc-pal.h	Declare `ds_ipc_stream_poll`
src/native/eventpipe/ds-ipc-pal-websocket.c	Implement WebSocket poll func and update vtable
src/native/eventpipe/ds-ipc-pal-types.h	Remove old `DiagnosticsIpcPollEvents` enum
src/native/eventpipe/ds-ipc-pal-socket.c	Add socket poll func and vtable entry
src/native/eventpipe/ds-ipc-pal-namedpipe.c	Stub out named pipe poll func
src/native/eventpipe/ds-eventpipe-protocol.c	Always pass `ds_ipc_stream_get_stream_ref(stream)`

src/native/eventpipe/ep-session.c

src/native/eventpipe/ep-ipc-stream.h

src/native/eventpipe/ds-ipc-pal-socket.c

src/native/eventpipe/ds-ipc-pal-namedpipe.c

src/native/eventpipe/ds-eventpipe-protocol.c

src/native/eventpipe/ds-ipc-pal-namedpipe.c

src/native/eventpipe/ds-ipc-pal-socket.c

src/native/eventpipe/ds-ipc-pal-websocket.c

src/native/eventpipe/ds-ipc-pal-namedpipe.c

src/native/eventpipe/ep-stream.c

noahfalk · 2025-07-10T00:22:17Z

src/native/eventpipe/ep-stream.h

@@ -355,5 +355,26 @@ ep_ipc_stream_writer_write (
 	uint32_t bytes_to_write,
 	uint32_t *bytes_written);

+/*
+ * IpcContinuationStream


Defining a new struct to wrap IpcStream seems unnecessary. Couldn't we just make the continuation_stream field in the session be type IpcStream*?

Right, we can. I just wasn't sure if IpcStream was meant to only be a base class that should be derived for different implementations, like how the different PAL implementations of DiagnosticsIpcStream all just derive from IpcStream.

I think IpcStream is meant to be a base class for different types of stream implementations, for example socket stream vs named pipe stream vs websocket stream. Its a bit less obvious because even though derived types have different implementations they are all reusing the same 'DiagnosticsIpcStream' type name. At some point in the future we could rename them SocketIpcStream, NamedPipeIpcStream, etc to make the inheritance pattern clearer. Sidestepping the naming, unless we are implementing a new transport I wouldn't expect a new stream derived type is needed.

src/native/eventpipe/ep-session.h

src/native/eventpipe/ep-session.c

src/native/eventpipe/ds-ipc-pal-types.h

Revert moving IpcPollEvents to ep-ipc-pal-types header Use IpcStream base type directly in sessions Omit websocket ipc_stream_poll_func implementation Change continuation stream poll timeout to infinite Change returned PollEvent to none upon timeout

ep-stream.h for ep_ipc_stream_writer_alloc that was transitively included by ep-file.h ep-ipc-stream.h for ep_ipc_stream_poll_vcall

lateralusX · 2025-07-11T07:21:41Z

src/native/eventpipe/ep-session.h

@@ -71,6 +71,9 @@ struct _EventPipeSession_Internal {
 	volatile uint32_t ref_count;
 	// The user_events_data file descriptor to register Tracepoints and write user_events to.
 	int user_events_data_fd;
+	// The IPC continuation stream from initializing the session through the diagnostic server
+	// Currently only initialized for user_events sessions.
+	IpcStream *stream;


We don't need to do it in this PR, but going forward I believe we should do different versions of this struct depending on the type and move specific fields into sub types instead of having a fields in one struct where type dictates what fields is applicable without any further information.

Right, I think we can have a Refactoring PR where we split up the versions of the struct, also migrate the IPC types to its own file, and move the user_events_data fd into the Stream structure

So IPC namespace, Clarify the IpcStream inheritance naming, this, and the followup work from the main user_events PR (#115265 (comment) ,#115265 (comment), #115265 (comment))

src/native/eventpipe/ds-eventpipe-protocol.c

lateralusX · 2025-07-11T07:40:26Z

src/native/eventpipe/ep-ipc-pal-types.h

+	DS_IPC_POLL_EVENTS_HANGUP = 0x02, // connection remotely closed
+	DS_IPC_POLL_EVENTS_ERR = 0x04, // error
+	DS_IPC_POLL_EVENTS_UNKNOWN = 0x80 // unknown state
+} DiagnosticsIpcPollEvents;


I know it already have been discussed, but maybe we should just drop EP/DS from this so its "common" meaning IPC_POLL_EVENTS_*, IpcPollEvents, IPC_TIMEOUT_INFINITE and going forward we might put all the low level IPC stuff into its own IPC namespace and share that code cross components.

src/native/eventpipe/ep-session.c

Remove prefix from IpcPollEvents enums and infinite timeout define Expand connection closed detection to error case

mdh1418 requested review from noahfalk, lateralusX and Copilot July 8, 2025 20:28

github-actions bot added the area-Tracing-coreclr label Jul 8, 2025

dotnet-policy-service bot assigned mdh1418 Jul 8, 2025

Copilot AI reviewed Jul 8, 2025

View reviewed changes

mdh1418 and others added 2 commits July 8, 2025 20:43

[EventPipe][DiagnosticServer] Add poll interface to IpcStreams

a07870f

[EventPipe] Close user_events session upon continuation_stream closure

c7fc5e6

mdh1418 force-pushed the ep_user_events_close_session_on_stream_close branch from a529a85 to c7fc5e6 Compare July 8, 2025 20:43

This was referenced Jul 9, 2025

browser-wasm Windows build error #116746

Open

wasm build failure in CI #117017

Open

noahfalk reviewed Jul 10, 2025

View reviewed changes

mdh1418 and others added 2 commits July 10, 2025 19:54

Address Feedback

6eb3334

Revert moving IpcPollEvents to ep-ipc-pal-types header Use IpcStream base type directly in sessions Omit websocket ipc_stream_poll_func implementation Change continuation stream poll timeout to infinite Change returned PollEvent to none upon timeout

Move IpcPollEvents back to ep-ipc-pal-types header

37c0056

mdh1418 force-pushed the ep_user_events_close_session_on_stream_close branch from c131816 to 37c0056 Compare July 10, 2025 20:47

build-analysis bot mentioned this pull request Jul 10, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Add relevant headers

419ddf9

ep-stream.h for ep_ipc_stream_writer_alloc that was transitively included by ep-file.h ep-ipc-stream.h for ep_ipc_stream_poll_vcall

lateralusX reviewed Jul 11, 2025

View reviewed changes

src/native/eventpipe/ds-eventpipe-protocol.c Show resolved Hide resolved

lateralusX reviewed Jul 11, 2025

View reviewed changes

src/native/eventpipe/ep-session.c Outdated Show resolved Hide resolved

Address feedback

f3f19c7

Remove prefix from IpcPollEvents enums and infinite timeout define Expand connection closed detection to error case

noahfalk approved these changes Jul 14, 2025

View reviewed changes

This was referenced Jul 15, 2025

Test failure: baseservices/exceptions/stackoverflow/stackoverflowtester/stackoverflowtester.cmd #110173

Open

Timeout in HostFactoryResolverTests.NoSpecialEntryPointPatternCanRunInParallel #114704

Open

[EventPipe] End user_events session upon continuation_stream closure #117435

Are you sure you want to change the base?

[EventPipe] End user_events session upon continuation_stream closure #117435

Uh oh!

Conversation

mdh1418 commented Jul 8, 2025

Testing

Uh oh!

mdh1418 commented Jul 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noahfalk Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

mdh1418 Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

noahfalk Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lateralusX Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

mdh1418 Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

mdh1418 Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lateralusX Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!