-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NativeAOT-LLVM] support System.Net.Http.HttpClient
on WASIp2
#2614
Conversation
System.Net.Http.HttpClient
on WASIp2System.Net.Http.HttpClient
on WASIp2
TODO items:
|
Ah, that CI failure was useful; now I see how WASI tests are currently run. Looks like I'll need to make some changes to convert the test module(s) into component(s) and run them on a WASIp2-compatible runtime. |
src/libraries/System.Net.Http/src/System/Net/Http/WasiHttpHandler/WasiHttpHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/WasiHttpHandler/WasiHttpHandler.cs
Outdated
Show resolved
Hide resolved
140cece
to
d3cf163
Compare
8461ce7
to
bb76b64
Compare
Status update on this: CI is green, and I've manually tested AFAICT, CI currently only runs the library smoke tests for both the WASI and browser targets. I can see that e.g. I've tried running the So my question is: What's the best (read: most practical and efficient) way to add test coverage for Meanwhile, I'm going to mark this "ready for review" since I believe it's ready for feedback even with the testing question unresolved. |
@@ -165,7 +165,7 @@ public static int Main(string[] args) | |||
if (passed && CoreFXTestLibrary.Internal.Runner.NumPassedTests > 0) | |||
{ | |||
CoreFXTestLibrary.Logger.LogInformation("All tests PASSED."); | |||
return 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 100
to indicate success is a convention used by all tests under src/tests. Changing this convention in the wasm branch is going to be perpetual conflict with upstream. Is this change really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I followed @akoeplinger 's advice to print the exit code to standard out, but then @SingleAccretion advised me to change the codes instead. I'm happy to do whatever you all think is best here.
For context: the reason we need to do something special for WASIp2 here is WebAssembly/wasi-cli#11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for xharness solution rather than editing 100
in too many places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see how you can avoid modifying the tests that are Main
-based. There is no code to be hooked there (unless you start thinking of some really involved workaround like making everything use CustomMain
).
If we need to modify the tests regardless, the simplest thing is to change the exit code.
(Actually, I wanted to probe grounds for doing this upstream as well, but it may be problematic.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just modify the native C main to printf exit code ?
Perhaps only when the application is build with some flags on, so that production applications don't do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just modify the native C main to printf exit code ?
Perhaps only when the application is build with some flags on, so that production applications don't so it.
I have not seen a precedent for adding such test hooks into production code.
It wouldn't work for cases that exit abnormally (Environment.Exit
) - we have one such test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially for NAOT the application is recompiled every time, so #ifdef
seems OK to me.
For C# exit, we could have hook in PAL. But I'm not sure what to do about abort()
somewhere in native code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially for NAOT the application is recompiled every time, so #ifdef seems OK to me. For C# exit, we could have hook in PAL
The bootstrapper, which is where the native main
is defined, is only compiled once, as part of the runtime build. I agree it is technically possible to make it work (e. g. via clever use of weak symbols, or compiler intrinsics, or something else).
My point is that it would not be to the scale of the problem, which is just running these smoke tests. If we don't convert all tests to use the zero exit code (unlikely), or don't wait enough for WASI P2.1 to add support for exit codes (possible but not terribly likely), then, when the question of how to run all of the runtime tests comes up - which will be upstream, it will be solved in some more involved manner. At that time, the same solution, if technically possible, will be adopted downstream too.
Edit: we'll be getting functional exit codes in a few months: WebAssembly/wasi-cli#44.
Yeah, enabling more tests should be independent PR. It takes non-trivial effort in the first pass. Also because at the same time it will test host's HTTP stack and interop, including HTTP edge cases.
That's one we want to use but we only use tests which are fully async. There is
The HTTP server role is played by Also note, that some of the tests assume HTTP server in the same process. Things which are broken should be marked with filter for broken scenario and a link to GH issue for it.
|
...libraries/System.Net.Http/src/System/Net/Http/WasiHttpHandler/WasiHttpWorld_component_type.o
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/WASIp2/generate-bindings.sh
Outdated
Show resolved
Hide resolved
src/tests/nativeaot/SmokeTests/SharedLibrary/LibraryWorld_cabi_realloc.o
Outdated
Show resolved
Hide resolved
111f0b8
to
aecff25
Compare
This adds `WasiHttpHandler`, a new implementation of `HttpMessageHandler` based on [wasi:http/outgoing-handler](https://github.com/WebAssembly/wasi-http/blob/v0.2.0/wit/handler.wit), plus tweaks to `System.Threading` to allow async `Task`s to work in a single-threaded context, with `ThreadPool` work items dispatched from an application-provided event loop. WASIp2 supports asynchronous I/O and timers via `wasi:io/poll/pollable` resource handles. One or more of those handles may be passed to `wasi:io/poll/poll`, which will block until at least one of them is ready. In order to make this model play nice with C#'s `async`/`await` and `Task` features, we need to reconcile several constraints: - WASI is currently single-threaded, and will continue to be that way for a while. - C#'s `async`/`await` and `Task` features require a working `ThreadPool` implementation capable of deferring work. - A WASI component can export an arbitrary number of functions to the host, and though they will always be called synchronously from the host, they need to be able to perform asynchronous operations before returning. - WASIp3 (currently in the design and prototype phase) will support asynchronous exports, with the top level event loop running in the host instead of the guest, and `wasi:io/poll/pollable` will no longer exist. Therefore, we don't want to add any temporary public APIs to the .NET runtime which will become obsolete when WASIp3 arrives. The solution we arrived at looks something like this: - Tweak the existing `ThreadPool` implementation for WASI so that methods such as `RequestWorkerThread` don't throw `PlatformNotSupportedException`s (i.e. allow work items to be queued even though the "worker thread" is always the same one that is queuing the work) - Add two new methods to `Thread`: - `internal static void Dispatch`: Runs an iteration of event loop, draining the `ThreadPool` queue of ready work items and calling `wasi:io/poll/poll` with any accumulated `pollable` handles - `internal static Task Register(int pollableHandle)`: Registers the specified `pollable` handle to be `poll`ed during the next call to `Dispatch` - Note that these methods are `internal` because they're temporary and should not be part of the public API, but they are intended to be called via `UnsafeAccessor` by application code (or more precisely, code generated by `wit-bindgen` for the application) The upshot is that application code can use `wit-bindgen` (either directly or via the new `componentize-dotnet` package) to generate async export bindings which will provide an event loop backed by `Thread.Dispatch`. Additionally, `wit-bindgen` can transparently convert any `pollable` handles returned by WASI imports into `Task`s via `Thread.Register`, allowing the component to `await` them, pass them to a combinator such as `Task.WhenEach`, etc. Later, when WASIp3 arrives and we update the .NET runtime to target it, we'll be able to remove some of this code (and the corresponding code in `wit-bindgen`) without requiring significant changes to the application developer's experience. This PR contains a few C# source files that were generated by `wit-bindgen` from the official WASI WIT files, plus scripts to regenerate them if desired. Signed-off-by: Joel Dice <joel.dice@fermyon.com> switch to `wasm32-wasip2` and update WASI test infra Now that we're using WASI-SDK 22, we can target `wasm32-wasip2`, which produces components by default and includes full `wasi:sockets` support. In order to run those components, I've updated the test infrastructure to use Wasmtime 21 (the latest release as of this writing). Other changes of note: - Tweaked src/coreclr/jit/compiler.cpp to make `Debug` builds work on Linux - Added libWasiHttp.a, which includes the encoded component type (in the form of a pre-generated Wasm object file) and a `cabi_realloc` definition. Both of these are generated by `wit-bindgen` and required by `wasm-component-ld` to generate a valid component. - Added a `FindWasmHostExecutableAndRun.sh` script for running the WASI tests on UNIX-style platforms. Signed-off-by: Joel Dice <joel.dice@fermyon.com> various WASI build tweaks Signed-off-by: Joel Dice <joel.dice@fermyon.com> quote libWasiHttp.a path in custom linker arg Signed-off-by: Joel Dice <joel.dice@fermyon.com> fix wasm-component-ld download command Signed-off-by: Joel Dice <joel.dice@fermyon.com> tweak EmccExtraArgs in CustomMain.csproj so wasm-component-ld understands it Signed-off-by: Joel Dice <joel.dice@fermyon.com> update CMake minimum version in wasi-sdk-p2.cmake Signed-off-by: Joel Dice <joel.dice@fermyon.com> use `HeaderDescriptor` to sort content and response headers Signed-off-by: Joel Dice <joel.dice@fermyon.com> allow building native WASI test code in src/tests/build.sh Signed-off-by: Joel Dice <joel.dice@fermyon.com> allow WASI runtime tests to be built and run on non-Windows systems Signed-off-by: Joel Dice <joel.dice@fermyon.com> update runtime tests to work with WASIp2 As of this writing, WASIp2 [does not support process exit statuses](WebAssembly/wasi-cli#11) beyond 0 (success) and 1 (failure). To work around this, I've modified the relevant tests to return 0 on success instead of 100. Signed-off-by: Joel Dice <joel.dice@fermyon.com> fix CI for Windows builds; remove unused file Signed-off-by: Joel Dice <joel.dice@fermyon.com> disable sprintf warning in llvmlssa.cpp on macOS Signed-off-by: Joel Dice <joel.dice@fermyon.com> remove LibraryWorld_cabi_realloc.o I didn't mean to add this to Git. Signed-off-by: Joel Dice <joel.dice@fermyon.com> rename `generate-bindings.sh` files for clarity This makes it more obvious that, though they are similar, they each have a different job. Signed-off-by: Joel Dice <joel.dice@fermyon.com> update to `wit-bindgen` 0.27.0 and regenerate bindings Signed-off-by: Joel Dice <joel.dice@fermyon.com> reorganize code; add HttpClient smoke test - move System/WASIp2 to System/Threading/WASIp2 - remove generated `cabi_realloc` functions since `wasi-libc` will provide one - add HttpClient test to SmokeTests/SharedLibrary Note that I put the HttpClient test in SmokeTests/SharedLibrary since we were already using NodeJS for that test, and adding a simple loopback webserver to SharedLibraryDriver.mjs was easiest option available to keep the whole test self-contained. Signed-off-by: Joel Dice <joel.dice@fermyon.com> implement SystemNative_SysLog for WASI Signed-off-by: Joel Dice <joel.dice@fermyon.com> increase NodeJS stack trace limit to 200 Signed-off-by: Joel Dice <joel.dice@fermyon.com> give guest no filesystem access in SharedLibraryDriver.mjs Signed-off-by: Joel Dice <joel.dice@fermyon.com> switch to Trace.Assert into HttpClient smoke test Signed-off-by: Joel Dice <joel.dice@fermyon.com> rename WASIp2 directory to Wasi Signed-off-by: Joel Dice <joel.dice@fermyon.com> fix non-GET methods and add HttpClient echo test Signed-off-by: Joel Dice <joel.dice@fermyon.com> use azure NPM rename - WasiEventLoop.RegisterWasiPollable - WasiEventLoop.DispatchWasiEventLoop to make it less confusing on the Thread class - unification of gen-buildsys - cleanup pal_process_wasi.c fix build? more buffer /echo request body in SharedLibraryDriver.mjs Signed-off-by: Joel Dice <joel.dice@fermyon.com> fix gen-buildsys.sh regression Signed-off-by: Joel Dice <joel.dice@fermyon.com> allow only infinite `HttpClient.Timeout`s on WASI This temporary code will be reverted once we support `System.Threading.Timer` on WASI in a forthcoming PR. Signed-off-by: Joel Dice <joel.dice@fermyon.com> use `&` operator to simplify install-jco.ps1 Signed-off-by: Joel Dice <joel.dice@fermyon.com> remove redundant `CheckWasmSdks` target from SharedLibrary.csproj Signed-off-by: Joel Dice <joel.dice@fermyon.com> split `FindWasmHostExecutable.sh` out of `FindWasmHostExecutableAndRun.sh` Signed-off-by: Joel Dice <joel.dice@fermyon.com> replace component type object files with WIT files This updates `wit-bindgen` and `wasm-component-ld`, which now support producing and consuming component type WIT files as an alternative to binary object files. These files are easier to audit from a security perspective. Signed-off-by: Joel Dice <joel.dice@fermyon.com> preserve slashes in path in SharedLibrary.csproj Signed-off-by: Joel Dice <joel.dice@fermyon.com> temporarily disable ThreadPoolWorkQueue.Dispatch assertion See dotnet/runtime#104803 Signed-off-by: Joel Dice <joel.dice@fermyon.com> update `wit-bindgen` to version 0.28.0 Signed-off-by: Joel Dice <joel.dice@fermyon.com> upgrade to wasi-sdk 24 and wit-bindgen 0.29.0 Signed-off-by: Joel Dice <joel.dice@fermyon.com> check for WASI in `PhysicalFileProvider.CreateFileWatcher` Signed-off-by: Joel Dice <joel.dice@fermyon.com> switch back to WASI 0.2.0 0.2.1 is not yet widely supported, and causes [trouble](bytecodealliance/jco#486) for Jco, which rely on for the `SharedLibrary` test. Signed-off-by: Joel Dice <joel.dice@fermyon.com> remove use of `WeakReference` from `WasiEventLoop` This was causing `HttpClient` timeout tests in the `SharedLibrary` smoke test suite to fail, apparently due to `TimerQueue.SetNextTimer` calling `WasiEventLoop.RegisterWasiPollable`, attaching a continuation to the resulting `Task` and then letting go of the reference, allowing it to be GC'd. Signed-off-by: Joel Dice <joel.dice@fermyon.com> skip unsupported signal handling on WASI Signed-off-by: Joel Dice <joel.dice@fermyon.com> throw PlatformNotSupportedException in ManualResetEventSlim.Wait on WASI Otherwise, we end up in an infinite loop. Signed-off-by: Joel Dice <joel.dice@fermyon.com> Revert "switch back to WASI 0.2.0" This reverts commit a8608b4. enable `NameResolution` and `Sockets` on WASI Signed-off-by: Joel Dice <joel.dice@fermyon.com> set `SocketsHttpHandler.IsEnabled` to `false` on WASI ...at least until we get `System.Net.Sockets` working. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
This requires passing the `CancellationToken` to `WasiEventLoop` and checking it before polling. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
aecff25
to
28d758f
Compare
Sometimes there are two Wasm files in Jco's output; sometimes three. In any case, hard-coding the number won't fly. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
// For WASI/browser/iOS/tvOS we will proactively fallback to polling since FileSystemWatcher is not supported. | ||
if (OperatingSystem.IsWasi() || OperatingSystem.IsBrowser() || (OperatingSystem.IsIOS() && !OperatingSystem.IsMacCatalyst()) || OperatingSystem.IsTvOS()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most library changes here should be upstreamed (we will merge them here before that, but it should only be temporary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have many such changes in messy PR here dotnet/runtime#105838
I don't expect to merge it in current form.
The problem is that attributes are visible on the runtime API, so we better do it right on the first attempt.
But I don't know exactly what would be PNSE and what would eventually work. And that will change with next WASI preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit I've been using this PR as a dumping ground for miscellaneous WASI fixes as I build demo apps (this one came up when testing AspNetCore). Happy to open separate, upstream PRs as appropriate, but I figured I'd collect them all here first, at least temporarily.
@pavelsavara: yeah, I've been avoiding attributes for the time being since I also don't know which features might be supported in the future. I'm pretty sure WASI will never support signal handling, but I could imagine it might support file notification someday.
@@ -462,7 +462,7 @@ | |||
</ItemGroup> | |||
|
|||
<!-- TODO-LLVM: This is not upstreamable because it makes the build runtime-specific. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- TODO-LLVM: This is not upstreamable because it makes the build runtime-specific. --> |
There is another one above (line 27) that needs to be deleted.
// TODO-LLVM: This is not upstreamable and should be deleted when https://github.com/dotnet/runtimelab/pull/2614 is merged | ||
#if TARGET_WASI && !NATIVE_AOT | ||
#if TARGET_WASI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the TODOs here should be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm rewriting the whole file upstream to respect child resources and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if the TODO-LLVM
comments were still relevant (i.e. did they refer to just the && !NATIVE_AOT
part, or the whole #if
conditional?) Sounds like they're no longer relevant, so I'll remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were introduced very recently in https://github.com/dotnet/runtimelab/pull/2605/files. I assumed it was a fix for some build break.
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
See bytecodealliance/wit-bindgen#1040 Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Otherwise, we end up overwriting `response.Content.Headers`. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
If one or more tasks have been canceled during the call to `ThreadPoolWorkQueue.Dispatch`, one or more tasks of interest to the application may have completed, so we return control immediately without polling, allowing the app to exit if it chooses. A practical example of this is in the SharedLibrary smoke test. Without this patch, that test will take over 100 seconds to complete, whereas with this patch it completes in under a second. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
c1fd2a7
to
98bf4dc
Compare
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
...and hopefully make CI happier. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
I'm still investigating whether this actually _is_ a `wasmtime-wasi-http` bug; stay tuned. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
I rebased this PR into #2758 |
This adds
WasiHttpHandler
, a new implementation ofHttpMessageHandler
based onwasi:http/outgoing-handler, plus tweaks to
System.Threading
to allow asyncTask
s to work in a single-threaded context, withThreadPool
work items dispatched from an application-provided event loop.WASIp2 supports asynchronous I/O and timers via
wasi:io/poll/pollable
resource handles. One or more of those handles may be passed towasi:io/poll/poll
, which will block until at least one of them is ready. In order to make this model play nice with C#'sasync
/await
andTask
features, we need to reconcile several constraints:async
/await
andTask
features require a workingThreadPool
implementation capable of deferring work.wasi:io/poll/pollable
will no longer exist. Therefore, we don't want to add any temporary public APIs to the .NET runtime which will become obsolete when WASIp3 arrives.The solution we arrived at looks something like this:
ThreadPool
implementation for WASI so that methods such asRequestWorkerThread
don't throwPlatformNotSupportedException
s (i.e. allow work items to be queued even though the "worker thread" is always the same one that is queuing the work)Thread
:internal static void Dispatch
: Runs an iteration of the event loop, draining theThreadPool
queue of ready work items and callingwasi:io/poll/poll
with any accumulatedpollable
handlesinternal static Task Register(int pollableHandle)
: Registers the specifiedpollable
handle to bepoll
ed during the current or next call toDispatch
internal
because they're temporary and should not be part of the public API, but they are intended to be called viaUnsafeAccessor
by application code (or more precisely, code generated bywit-bindgen
for the application)The upshot is that application code can use
wit-bindgen
(either directly or via the newcomponentize-dotnet
package) to generate async export bindings which will provide an event loop backed byThread.Dispatch
. Additionally,wit-bindgen
can transparently convert anypollable
handles returned by WASI imports intoTask
s viaThread.Register
, allowing the component toawait
them, pass them to a combinator such asTask.WhenEach
, etc.Later, when WASIp3 arrives and we update the .NET runtime to target it, we'll be able to remove some of this code (and the corresponding code in
wit-bindgen
) without requiring significant changes to the application developer's experience.This PR contains a few C# source files that were generated by
wit-bindgen
from the official WASI WIT files, plus scripts to regenerate them as desired.