[wasm-ep] Use DOTNET_DiagnosticPorts to configure Diagnostic Server #73370
Conversation
lambdageek
added
arch-wasm
|
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsParse it the same way that the C code does:
Additionally, move Fixes #73011
|
| @@ -329,6 +331,10 @@ async function mono_wasm_after_user_runtime_initialized(): Promise<void> { | |||
|
|
|||
| if (runtimeHelpers.diagnosticTracing) console.debug("MONO_WASM: Initializing mono runtime"); | |||
|
|
|||
| if (MonoWasmThreads) { | |||
There was a problem hiding this comment.
maybe you could now move it earlier in the startup, just after beforeOnRuntimeInitialized.promise_control.resolve. but I'm not sure it's useful.
There was a problem hiding this comment.
I'll move it. the only hard requirement is that the diagnostic server has to start before we call into driver.c to start the root domain initialization. Starting later is better
There was a problem hiding this comment.
Ah, there's an annoying problem here. In the non-Blazor startup we don't set environment variables until _apply_configuration_from_args. So actually I have to try to init diagnostics in two places.
|
Please also update |
|
@pavelsavara @maraf I'd like to get #73305 in before this one. I can't really test what's here until that other PR goes on. @maraf I think I want to delete of |
lambdageek
force-pushed
the
wasm-ep-setenv
branch
from
81ad8ce to
038e6dc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Parse it the same way that the C code does: ``` <uri>[,<connect|listen>][,<suspend|nosuspend>] ``` - uri should be a websocket uri - listen is not supported as it doesn't make sense with a WebSocket - connect is the default if omitted - suspend is the default if omitted --- Additionally, move `mono_wasm_diagnostics_init` to later in the startup flow. This gives Blazor a chance to set DOTNET_DiagnosticPorts from their `onRuntimeInitialized` callback. Fixes dotnet#73011
…lazor It has to be after environment variables are set, but before mono_wasm_load_runtime is called. There is no good place that's common to both startup paths. Try it on both. Use a flag to make diagnostics initialization run at most once
lambdageek
force-pushed
the
wasm-ep-setenv
branch
from
038e6dc to
4f5ab15
Compare
|
/azp run runtime-wasm |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-wasm |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| export async function mono_wasm_init_diagnostics(options: DiagnosticOptions): Promise<void> { | ||
| let diagnosticsInitialized = false; | ||
|
|
||
| export async function mono_wasm_init_diagnostics(opts: "env" | DiagnosticOptions): Promise<void> { |
There was a problem hiding this comment.
is the signature with DiagnosticOptions param type still useful ?
There was a problem hiding this comment.
No. I want to do a follow up PR to simplify more of the diagnostics support. removing this argument is just one piece.
ghost
locked as resolved and limited conversation to collaborators
Parse it the same way that the C code does:
Additionally, move
mono_wasm_diagnostics_initto later in thestartup flow. This gives Blazor a chance to set
DOTNET_DiagnosticPortsfrom theironRuntimeInitializedcallback.An unfortunate problem here is that blazor and non-blazor startup need to call diagnostics init from different places because of how environment variables are populated. So we try to init diagnostics twice and set a flag.
Fixes #73011