Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #115.
The current draft de-unifies the ways we call
_routes
and the handler entrypoint, because one is convenient to have as a vector in a blocking way, while the other we want as a stream to hand off tohyper
to hook up to the HTTP response network stream. It would be desirable to at least partially re-unify these.The key component here is the
StreamWriter
type (which should have a better name). This exists because we need something that is Write - so that WASI WritePipe can wrap it - but can also be Stream so thathyper::Body
can stream it. This is gnarly because Write is strictly synchronous.The current solution runs the Wasm module (and hence the Write calls) in a child task, and then uses async-stream over a polling loop to present the content as a stream in the main request handling task.If the phrase "polling loop" gives you jitters, I'm with you. I have left it some comments showing how I tried to have the async runtime wake the task when new data was written. But what this ended up doing was serving some of the response, then blocking waiting for a change notification until Wasm finished writing. So it looked like you were getting a quick time to first byte, but your time to, say, hundred and first was not so good. I would really like to understand what was going on here and rework it to avoid the polling; I'm partly anxious about whether the polling will cause undue load when there are a lot of requests, but also it is just so clearly not right.The solution to this, which is probably not ideal but seems like an adequate compromise after banging my head against tokio for several days, is as follows:
Why all the waiting for fractions of microseconds? Well, in my tests, if I simply went straight to "wait for change notification," the first few waits would work fine, but then something would stall, and it would wait until the end of input. This caused a very noticeable hiccup in the HTTP response. I haven't been able to work out why it stalled - if anyone has any insight, I would love to learn from you! But adding those tiny waits allowed the pipeline to keep flowing.
Incidentally, the tiny waits alone were enough, but I was concerned that they would result in a lot of no-op polls of the buffer. Checking the change notification means that if stuff is piling up quickly if will be served (the wait-for-change will process almost synchronously), but we won't hammer the loop if there's nothing to do.
We should absolutely bear this in mind when we load test WAGI with large numbers of requests. It will important to include large and slow responses in that testing.
There is also some error handling that needs tidying up.I think this is now done.On the plus side, large or slow responses start arriving a lot faster now. Here are the curl stats from before, for a simple guest that just prints 100000 lines of the form
<p>Line 123</p>
:And her are the stats with this PR applied (debug on my local machine, so highly unscientific):