-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Use the same UdpSocket in WASIp{2,3}
#11384
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
Use the same UdpSocket in WASIp{2,3}
#11384
Conversation
| socket: Arc::new(socket), | ||
| udp_state: UdpState::Default, | ||
| family: socket_address_family, | ||
| socket_addr_check: None, |
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 clone the "check" from context here and avoid having to Arc::clone on each p3 usage of it? (#11291 (comment))
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.
Unfortunately no because the check will live in the store no matter what and we can't borrow the store across awaits, so it's got to get moved out. This is mostly just here to handle how in WASIp2 the socket check is inherited from the Network type rather than unconditionally from WasiCtx. While esoteric it's possible for embedders to push their own Network without using WasiCtx which has a distinct check than the one in WasiCtx itself, although we might want to remove such a feature eventually (which would remove the need for this field here)
This commit refactors the implementation of `wasi:sockets` for WASIp2 and WASIp3 to use the same underlying host data structure for the `UdpSocket` resource in WIT. Previously each version of WASI had its own socket which resulted in duplicated code. There's some minor differences between WASIp2 and WASIp3 but it's easy enough to paper over with the same socket type. This is intended to help with the maintainability of this going forward to only have one type to operate on rather than two (which also ensures that bugfixes for one should affect the other). One other change made in this commit is that sprinkled checks for whether or not UDP is allowed are all removed and canonicalized during UDP socket creation. This means that UDP socket creation is the only location that checks for whether UDP is allowed. Once a UDP socket is created it can be used freely regardless of whether the UDP setting is enabled or disabled. This is not intended to have a large practical effect but it does mean the behavior of hosts that deny UDP but manually give access to a UDP socket resource to a component may behave subtly differently.
70ac013 to
ff093a8
Compare
This is the same as bytecodealliance#11384 except for `TcpSocket` to share the implementation of TCP across WASIp{2,3} and avoid duplicating code between the two.
* Use the same `TcpSocket` in WASIp{2,3}
This is the same as #11384 except for `TcpSocket` to share the
implementation of TCP across WASIp{2,3} and avoid duplicating code
between the two.
* Fix CI issues
* Review comments
* Trim "TCP allowed?" checks
Like UDP only perform the check on socket creation, not after it's
created.
* Use the same `UdpSocket` in WASIp{2,3}
This commit refactors the implementation of `wasi:sockets` for WASIp2
and WASIp3 to use the same underlying host data structure for the
`UdpSocket` resource in WIT. Previously each version of WASI had its own
socket which resulted in duplicated code. There's some minor differences
between WASIp2 and WASIp3 but it's easy enough to paper over with the
same socket type. This is intended to help with the maintainability of
this going forward to only have one type to operate on rather than two
(which also ensures that bugfixes for one should affect the other).
One other change made in this commit is that sprinkled checks for
whether or not UDP is allowed are all removed and canonicalized during
UDP socket creation. This means that UDP socket creation is the only
location that checks for whether UDP is allowed. Once a UDP socket is
created it can be used freely regardless of whether the UDP setting is
enabled or disabled. This is not intended to have a large practical
effect but it does mean the behavior of hosts that deny UDP but manually
give access to a UDP socket resource to a component may behave subtly
differently.
* Review comments
* Fix p3-less warnings
* Update UDP denial test
* Fix some clippy issues
* Fix no-udp test warnings
* Use the same `TcpSocket` in WASIp{2,3}
This is the same as bytecodealliance#11384 except for `TcpSocket` to share the
implementation of TCP across WASIp{2,3} and avoid duplicating code
between the two.
* Fix CI issues
* Review comments
* Trim "TCP allowed?" checks
Like UDP only perform the check on socket creation, not after it's
created.
This commit refactors the implementation of
wasi:socketsfor WASIp2 and WASIp3 to use the same underlying host data structure for theUdpSocketresource in WIT. Previously each version of WASI had its own socket which resulted in duplicated code. There's some minor differences between WASIp2 and WASIp3 but it's easy enough to paper over with the same socket type. This is intended to help with the maintainability of this going forward to only have one type to operate on rather than two (which also ensures that bugfixes for one should affect the other).One other change made in this commit is that sprinkled checks for whether or not UDP is allowed are all removed and canonicalized during UDP socket creation. This means that UDP socket creation is the only location that checks for whether UDP is allowed. Once a UDP socket is created it can be used freely regardless of whether the UDP setting is enabled or disabled. This is not intended to have a large practical effect but it does mean the behavior of hosts that deny UDP but manually give access to a UDP socket resource to a component may behave subtly differently.