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.
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
Document error codes. #18
Document error codes. #18
Changes from all commits
0ed510d
58c2a39
b45f877
ba6a42d
590e63a
0232ce6
d281396
b9a8557
a3c487b
19fcb3a
c3fd903
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Should there be a code corresponding to
ECONNABORTED
?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.
AFAIK,
ECONNABORTED
is only returned byaccept
. Which, in my opinion should not be forwarded into WASI land. This is documented further along in the document: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.
This is a little surprising. Indeed many users will want to just ignore
ECONNABORTED
and keep waiting for the next connection, but some users may want to know that the failure occurred so that they can log it. Large numbers of reset connections could be a sign of a network problem or unexpected client behavior. Some users may expect specific connections and want to know that a connection was attempted and reset so that they don't continue to wait for 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.
The choice of skipping transient errors stems from before the Preview2 PR. Before that,
accept
returned a singlestream
of TCP sockets, that would only close after a permanent error (if ever). This is a design avenue I'd still like to explore for Preview3.As you noted, there are indeed valid use cases for receiving these transient errors. It just felt too niche to make it into the first version of the proposal.
I think it can even be added afterwards, without breaking the current setup, like this:
then the libc implementation of
accept
can callpoll-oneof([accept-stream, error-stream])
.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 can see the appeal of a design like
get-errors
, but worry it would create awkward new surface area. How would it work if the user polls on just theerror-stream
and not theaccept-stream
? The underlying host APIs don't have any way to reportECONNABORTED
without actually trying to accept a connection.However, I can see the logic of deferring this for now. I've filed #22 to track this, and we can proceed with this PR.
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.
Would it make sense to move resolver errors to a separate enum? At a glance, the only code they share with the socket errors is
out-of-memory
.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.
They also share:
address-family-not-supported
access-denied
not-supported
resource-limit-reached
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.
When would a resolver API return
access-denied
? When would it returnnot-supported
?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.
Examples:
access-denied
: The network does not allow you to look up the specificname
.not-supported
: The network does not support name resolution.I don't know if operation systems ever return these error codes natively.
But in WASI, with its increased focus on sandbox-ability and virtualization, it makes sense to me to give embedders an escape hatch in case they don't want to support/give access to a specific function in an interface. This applies in general, not just the resolve API.