Support invoking wasi:http/incoming-handler #1249

mschuwalow · 2025-01-18T17:04:36Z

resolves #1184

mschuwalow · 2025-01-18T17:05:29Z

golem-common/src/model/exports.rs

@@ -16,24 +16,24 @@ use golem_wasm_ast::analysis::{AnalysedExport, AnalysedFunction, AnalysedInstanc

 use rib::{ParsedFunctionName, ParsedFunctionReference, ParsedFunctionSite};

-pub trait AnalysedExportExtensions {


These were unused

golem-worker-executor-base/src/virtual_export_compat/http_incoming_handler.rs

afsalthaj · 2025-01-19T03:12:12Z

golem-worker-executor-base/src/invocation.rs

+            .new_response_outparam(sender)
+            .unwrap();
+
+        unsafe {


I couldn't figure out from reading it the need of unsafe here. Would you mind explaining it here?

this comes from the async_scoped crate. https://docs.rs/async-scoped/latest/async_scoped/

The invariant they want here is just that:

An asynchronous function that creates a scope and immediately awaits the stream. The outputs of the futures are collected as a Vec and returned along with the output of the block. Safety This function is not completely safe: please see cancellation_soundness in [tests.rs](https://github.com/rmanoka/async-scoped/blob/master/src/tests.rs) for a test-case that suggests how this can lead to invalid memory access if not dealt with care. The caller must ensure that the lifetime ’a is valid until the returned future is fully driven. Dropping the future is okay, but blocks the current thread until all spawned futures complete.

Will add a comment.

golem-common/src/virtual_exports/http_incoming_handler.rs

vigoo · 2025-01-20T09:00:39Z

golem-common/src/virtual_exports/http_incoming_handler.rs

+        Ok(HttpBodyContent(Bytes::from(result)))
+    }
+
+    pub fn to_value(self) -> Value {


Instead of these to_value calls consider implementing the IntoValue type class (from wasm-rpc), that also requires you to specify the type, but it will allow us to use these in more scenarios (for example better integrating it PublicOplogEntry etc)

Annoyance here is that only one half of the types here need to go to values (the other half is being converted from values), while we need to have the type for all of them.

This would suggest that we need a counterpart of IntoValue that can go from values (FromValue?) and then we would likely want to split the type portions out from both.

I wanted to avoid doing this for now and just implemented the needed functionality as plain functions. We can always implement IntoValue if we end up needing it for a specific type.

vigoo · 2025-01-20T09:07:12Z

golem-worker-executor-base/src/invocation.rs

+
+    let mut store_context = store.as_context_mut();
+
+    let current_fuel_level = store_context.get_fuel().unwrap_or(0);


These post-invocation steps are now duplicated, I guess, maybe worth extracting them to a function

done, factored out into a new function

vigoo · 2025-01-20T09:13:19Z

golem-worker-executor-base/src/workerctx.rs

@@ -73,6 +74,7 @@ pub trait WorkerCtx:
    + Send
    + Sync
    + Sized
+    + DurableWorkerCtxView<Self>


This should not be necessary - and breaks the (now unused) purpose of this trait that it is independent of DurableWorkerCtx (that's just one implementation - originally we had a non-durable one too).

We already have as_wasi_http_view and as_wasi_view below, isn't that enough?

Also, Ctx decides whether worker context handles things related to durability. So this implies the above comment from @vigoo should be made work if it's not working now, in my view - which is simply .as_wasi_view where ever it is required.

vigoo · 2025-01-20T09:18:36Z

golem-worker-executor-base/tests/wasi.rs

+
+#[test]
+#[tracing::instrument]
+async fn wasi_incoming_request_handler_compat(


better test coverage please! :) (headers, body, etc)

Yes, this is coming 👍

Co-authored-by: Daniel Vigovszky <[email protected]>

vigoo · 2025-01-21T09:54:04Z

golem-worker-executor-base/src/invocation.rs

-    let res_or_error = match receiver.await {
+    let out = receiver.await;
+
+    tracing::warn!("received response {:?}", out);


Let's remove these debug warn!s (there is another one below too)

oh yes, sorry

vigoo

Found two debug logs that should be removed, otherwise good to go!

mschuwalow commented Jan 18, 2025

View reviewed changes

mschuwalow force-pushed the wasi-http-3 branch 2 times, most recently from 9541bf7 to bfd6632 Compare January 18, 2025 17:12

Support invoking wasi:http/incoming-handler

e99b2d1

mschuwalow force-pushed the wasi-http-3 branch from bfd6632 to e99b2d1 Compare January 18, 2025 17:13

afsalthaj reviewed Jan 19, 2025

View reviewed changes

golem-worker-executor-base/src/virtual_export_compat/http_incoming_handler.rs Show resolved Hide resolved

afsalthaj reviewed Jan 19, 2025

View reviewed changes

unsafety comment

8d2ed51

vigoo reviewed Jan 20, 2025

View reviewed changes

golem-common/src/virtual_exports/http_incoming_handler.rs Outdated Show resolved Hide resolved

vigoo reviewed Jan 20, 2025

View reviewed changes

mschuwalow and others added 5 commits January 20, 2025 13:01

Update golem-common/src/virtual_exports/http_incoming_handler.rs

7fb47f5

Co-authored-by: Daniel Vigovszky <[email protected]>

comments

432a381

test using wasm echo server

edc5133

Merge branch 'main' into wasi-http-3

14105cb

test restore of state when implementing wasi:http/incoming-handler

5b5e821

mschuwalow requested review from vigoo and afsalthaj January 20, 2025 17:27

vigoo reviewed Jan 21, 2025

View reviewed changes

vigoo approved these changes Jan 21, 2025

View reviewed changes

mschuwalow added 2 commits January 21, 2025 11:20

remove debug log

aff4c34

Merge branch 'main' into wasi-http-3

5e4150e

mschuwalow merged commit d22a30c into main Jan 21, 2025
20 checks passed

mschuwalow deleted the wasi-http-3 branch January 21, 2025 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support invoking wasi:http/incoming-handler #1249

Support invoking wasi:http/incoming-handler #1249

mschuwalow commented Jan 18, 2025

mschuwalow Jan 18, 2025

afsalthaj Jan 19, 2025

afsalthaj Jan 19, 2025

mschuwalow Jan 19, 2025 •

edited

Loading

vigoo Jan 20, 2025

mschuwalow Jan 20, 2025

vigoo Jan 20, 2025

mschuwalow Jan 20, 2025

vigoo Jan 20, 2025

afsalthaj Jan 20, 2025

mschuwalow Jan 20, 2025

vigoo Jan 20, 2025

mschuwalow Jan 20, 2025

mschuwalow Jan 20, 2025

vigoo Jan 21, 2025

mschuwalow Jan 21, 2025

vigoo left a comment

		@@ -16,24 +16,24 @@ use golem_wasm_ast::analysis::{AnalysedExport, AnalysedFunction, AnalysedInstanc

		use rib::{ParsedFunctionName, ParsedFunctionReference, ParsedFunctionSite};

		pub trait AnalysedExportExtensions {


		let mut store_context = store.as_context_mut();

		let current_fuel_level = store_context.get_fuel().unwrap_or(0);

Support invoking wasi:http/incoming-handler #1249

Support invoking wasi:http/incoming-handler #1249

Conversation

mschuwalow commented Jan 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschuwalow Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigoo left a comment

Choose a reason for hiding this comment

mschuwalow Jan 19, 2025 •

edited

Loading