abrown opened PR #8026 from abrown:serve-error-propagation
to bytecodealliance:main
:
The
wasmtime serve
command prints failed HTTP handling tostderr
but, when the reason was due to failure inside the HTTP handler itself, the printed error message is not descriptive enough. After looking at long dumps ofguest never invoked
response-outparam::setmethod
, I realized that errors inside the Tokiotask
are never propagated outwards. This change captures those errors and appends them to the printed error message.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
abrown requested fitzgen for a review on PR #8026.
abrown requested wasmtime-core-reviewers for a review on PR #8026.
abrown created PR review comment:
@elliottt, I guess if we adopt this PR we can remove this TODO line?
abrown submitted PR review.
pchickey requested pchickey for a review on PR #8026.
pchickey submitted PR review.
pchickey created PR review comment:
Yes, please remove this TODO and we can land this PR
pchickey submitted PR review:
Thanks for this fix!
elliottt submitted PR review:
Looks great to me, thanks for making this change @abrown!
elliottt submitted PR review:
Looks great to me, thanks for making this change @abrown!
elliottt created PR review comment:
// If the receiver has an error (`RecvError`), it only indicates that the task // exited before the sender was used, that the sender was dropped // before a response was sent. Instead we retrieve and
elliottt created PR review comment:
Do we need to bound the amount of time spent here, or are we guaranteed that the result should be available immediately because the
receiver.await
failed?
abrown updated PR #8026.
abrown updated PR #8026.
abrown submitted PR review.
abrown created PR review comment:
I kind of merged our two versions into 5a958a5; hopefully that still captures what you meant in this suggestion!
abrown created PR review comment:
As I looked at it, I concluded that yes, if the receiver failed it is because the task failed. But maybe I missed something or perhaps future changes will invalidate that. But in that case,
wasmtime serve
will be dumping errors to stderr anyways so it should be apparent to users that something is wrong apart from the (hypothetical) additional slowdown. And they'll probably care more about whatever caused the task to fail.
abrown submitted PR review.
abrown updated PR #8026.
abrown submitted PR review.
abrown created PR review comment:
Ok, added a comment about this assumption: f09760e
abrown updated PR #8026.
abrown edited PR review comment.
abrown has enabled auto merge for PR #8026.
alexcrichton merged PR #8026.
Last updated: Dec 23 2024 at 12:05 UTC