Add Reverse Diagnostics Server by josalem · Pull Request #33307 · dotnet/runtime

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem · 2020-03-06T22:28:05Z

This PR adds the ability for the runtime to connect to a pre-existing IPC Transport for the Diagnostics Server. This enables several interesting scenarios, including:

Many apps connecting to a single diagnostics agent
tracing via IPC at startup (imagine doing dotnet trace run <executable> ...)
A busybox-esque agent being configurable in a side-car container
A host-based agent running and tracing an application in a container

This is achieved, by adding the non-blocking Accept (think listen on Linux), Connect, and Poll APIs to the diagnostics PAL. Using these, the runtime is able to listen on the original IPC Transport it creates ($TMPDIR/dotnet-diagnostics-<pid>) and this reversed connection if configured.

This reversed connection is opt-in and only activates when a path is specified in the DOTNET_DiagnosticsMonitorAddress environment variable.

The following logic is almost entirely inside DiagnosticsIpcFactory::GetNExtAvailableStream(...)

If a user configures this mechanism, the runtime will infinitely attempt to connect the specified transport and is resilient to that transport closing and reopening. The retry logic is as follows:

Reverse connection is not configured => infinitely block on server connection
Reverse connection is configured
- Reverse connection is successful => cache connection, set timeout to max (30s)
- Reverse connection is unsuccessful or closed => set timeout to minimum (250ms) and poll on other connections, multiply by falloff factor (2) for each attempt until max (30s)

The runtime caches reverse connections so that it is not constantly attaching to the reverse transport every time poll times out or the server connection is used. These cache entries get cleared when a connection is used or the connection is hung up.

~~This PR is currently missing~~ The following will be a separate PR:

Semantics for blocking startup of the runtime
- To enable collecting startup events, we would add a configurable blocking timeout to runtime startup
- an environment variable will take a value of -1 (infinite), or 0 to n milliseconds
- this value will be used to determine how long to block eemain before an EventPipe session is started.
- the default will be 0 (no block)

CC - @tommcdon @shirhatti @noahfalk @sywhang

Dotnet-GitSync-Bot · 2020-03-06T22:28:09Z

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

noahfalk

Added a few comments and questions. Holding off on a more detailed review in expectation that things are still changing : )

noahfalk

Its shaping up but will still need some more work to be robust. Happy to chat if you've got questions about anything.

noahfalk · 2020-03-22T12:13:21Z

I would add an assert that that OVERLAPPED operation is complete. I would also test to confirm that DisconnectNamedPipe() finishes a ConnectNamedPipe operation when it is still pending. Destroying an OVERLAPPED struct before the operation ends usually causes painful to debug memory corruption issues.

* no select on unix * untested on unix

* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

* fix array alloc * properly cast array size

noahfalk

Tests and functional correctness looking way better! I still have a few requests on the API design, performance, and additional tests but nothing as critical as the last iteration.

* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

josalem · 2020-04-17T17:18:30Z

/azp run runtime

azure-pipelines · 2020-04-17T17:18:55Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk

Alright, things are starting to look ship shape 👍 I did find a few more minor potential issues, comments inline.

noahfalk · 2020-04-18T01:34:46Z

+ _pIpc->Close(callback);
+ if (_pStream != nullptr)
+ _pStream->Close(callback);
+ }


_pIpc and _pStream need to be deleted (not that a leak on shutdown would really matter, but may as well clean up properly)

I specifically leaked these since the PAL is unaware of shutdown paths and has access to these pointers. This method is specifically called from a different thread than the server thread. If we delete them in this code path it can lead to AVs on the server thread in the PAL (inside IpcStream::DiagnosticsIpc::Poll specifically). Rather than introduce some form of knowledge of runtime state to the PAL or potentially adding locks protecting access I opted to just leak the memory on shutdown, since that is the only place this method is called. I should add a comment that calls out this assumption, though.

josalem · 2020-04-20T06:32:39Z

Remaining failure appears to be an AzDO Package Feed or NuGet manifest error. All test runs passed.

This reverts commit 629dba5.

…t#35767)" This reverts commit 7a95cc8.

josalem added EventPipe area-Diagnostics-coreclr labels Mar 6, 2020

josalem added this to the 5.0 milestone Mar 6, 2020

josalem requested review from noahfalk and sywhang March 6, 2020 22:28

josalem self-assigned this Mar 6, 2020

noahfalk reviewed Mar 7, 2020
View reviewed changes

noahfalk mentioned this pull request Mar 12, 2020
[Feature request] dotnet-trace at startup dotnet/diagnostics#619
Closed
3 tasks

josalem marked this pull request as ready for review March 18, 2020 00:06

josalem requested a review from noahfalk March 18, 2020 00:06

josalem changed the title ~~[Draft] Add Reverse Diagnostics Server (client-mode)~~Add Reverse Diagnostics Server (client-mode)Mar 18, 2020

This was referenced Mar 19, 2020
Allow tools to specify a specific OS transport dotnet/diagnostics#770
Closed
Add DiagnosticsAgent API dotnet/diagnostics#931
Closed

noahfalk requested changes Mar 22, 2020
View reviewed changes

josalem force-pushed the dev/josalem/diag-server-handshake branch from 9f108d8 to 20f3745Compare April 1, 2020 22:41

John Salem added 14 commits April 2, 2020 14:59

Add Advertise IPC Command
bc21866

Add untested select connect and nonblocking accept
0255b37

Add select API on windows
7f8bb61
* no select on unix * untested on unix

Implement select for unix
4b97ca8

Update diagnostics server to use both modes
0cb1e88
* Change DOTNET_DiagnosticsServerAddress to DOTNET_DiagnosticsClientModeAddress * works for original connection mode * untested for client mode

Add DiagnosticsIpcFactory abstraction
dbc9624

Fix precondition
52cd9b0
* fix array alloc * properly cast array size

fix x86 build
3918060

Fixed errors
33da551

fix pipe connection on window
f7a28c4

fix gcc build
22e4476

fix gcc build
8204e70

Change environment variable name
a74f8a3

Remove unnecessary protocol code
6b1fe2a

noahfalk reviewed Apr 8, 2020
View reviewed changes

John Salem added 8 commits April 8, 2020 09:54

fix nits from review
469d425

Add ConnectionState abstraction
d93d846
* Adds ConnectionState class for hiding server/client diff * simplifies code for easier reading

Add test case
0fce7bc

Fix windows build
6e2f642

Fix test
18c9959
* test was creating a pipe with a 0 buffer * runtime needs to handle a 0 buffer namedpipe

Introduce a timeout to read/write
91d6d0e
* makes advertisement not block for more than 100 ms * TODO: implement on non-windows

Implement timeout read/write for unix
d4bf4c1

adjust poll timeout to be more aggressive
9311238

josalem requested a review from noahfalk April 17, 2020 22:18

noahfalk approved these changes Apr 18, 2020
View reviewed changes

John Salem added 6 commits April 19, 2020 18:41

Fix nits from review
b0e1218

simplify branching in GetIpcPollHandle
1cd8f29

Add 2 byte field to advertise
3530a25

Fix cast for advertize
67acd21

additional logging in tests
8c83f22

Loops unix send/recv to handle partial io
1cbb720

josalem changed the title ~~Add Reverse Diagnostics Server (client-mode)~~Add Reverse Diagnostics ServerApr 20, 2020

josalem merged commit 629dba5 into dotnet:masterApr 20, 2020

jaredpar mentioned this pull request Apr 21, 2020
Builds legs getting abandoned due to agent notification issues #35223
Closed

jkotas mentioned this pull request Apr 30, 2020
Test hang in System.Diagnostics.Tracing.EventProvider.EventUnregister #35451
Closed

jkotas added a commit that referenced this pull request May 3, 2020
Revert "Add Reverse Diagnostics Server (#33307)"
7fd1313
This reverts commit 629dba5.

jkotas mentioned this pull request May 3, 2020
Revert "Add Reverse Diagnostics Server" #35767
Merged

jkotas added a commit that referenced this pull request May 3, 2020
Revert "Add Reverse Diagnostics Server (#33307)" (#35767)
7a95cc8
This reverts commit 629dba5.

josalem pushed a commit to josalem/runtime that referenced this pull request May 7, 2020
Revert "Revert "Add Reverse Diagnostics Server (dotnet#33307)" (dotne…
e623063
…t#35767)" This reverts commit 7a95cc8.

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020