Stream: git-wasmtime

Topic: wasmtime / PR #8282 Separate TCP implementation from the ...

Wasmtime GitHub notifications bot (Apr 02 2024 at 13:02):

rylev opened PR #8282 from rylev:separate-system-tcp-impl2 to bytecodealliance:main:

Part of #7705 was moving the implementation of TCP (and UDP) out of the host bindings and into their own types. This PR starts this process (only for TCP) by doing a simple lift and shift of the code from the host bindings into the TcpSocket type. This should make next steps easier as the border between host bindings and implementation become much clearer with this change.

In the future, we may choose to make TcpSocket a trait object allowing users to swap in their own implementations, but that's left for a future change.

You may notice that some of TcpSocket's methods return io::Result while most return SocketResult. It would be nice to move TcpSocket over completely to io::Result so that the implementation is completely free of any host bindings. However, there are some unclear/missing mappings between io::Error and ErrorCode. For example, when the socket is in an invalid state, we currently return ErrorCode::InvalidState. However, there is no io::Error or Errno that maps 1 to 1 to this ErrorCode. We'll need to figure out exactly how we want to do this mapping, but I'm leaving that for a future PR.

Wasmtime GitHub notifications bot (Apr 02 2024 at 13:02):

rylev requested alexcrichton for a review on PR #8282.

Wasmtime GitHub notifications bot (Apr 02 2024 at 13:02):

rylev requested wasmtime-core-reviewers for a review on PR #8282.

Wasmtime GitHub notifications bot (Apr 02 2024 at 15:00):

rylev updated PR #8282.

Wasmtime GitHub notifications bot (Apr 02 2024 at 18:21):

alexcrichton commented on PR #8282:

Could you detail a bit more on where you envision this direction going? For example if all these methods were extracted into a trait I'm curious how that would differ from today where all the TCP interfaces are already behind traits, just wit-bindgen-generated ones. If the intention is to support an entirely custom TCP implementation without reusing much of the host one I'm wondering if it might be best to consider improving bindgen generation/options/etc instead? Before considering that too seriously though I'm curious to learn more about the trajectory of this.

Wasmtime GitHub notifications bot (Apr 02 2024 at 20:38):

rylev commented on PR #8282:

Certainly! You can see more of what I'm going for in #7705 (for example here).

While I think there will be a large overlap with the bindgen generated trait, there's going to be enough difference that trying to make it fit directly will be difficult (and will only grow more difficult over time). I tried to keep this PR as simple as possible which meant a pretty 1 to 1 lift and shift which admittedly makes it a bit hard to see where the difference between the concrete implementation and the bindgen generated trait is. However, I can imagine that some state management will move back into the host module and the concrete implementation will change before we (potentially) move towards making this a trait.

That all being said, I personally find the code as it is now easier to read since the actual networking code is separate from the boiler plate of resource management, and I'd argue even if no other abstraction were to take place, this would be a worthwhile refactor.

Wasmtime GitHub notifications bot (Apr 02 2024 at 21:26):

alexcrichton commented on PR #8282:

Yeah I would agree with you that in isolation I see no issue with this PR, I'm mostly curious about looking ahead and seeing where it's going. Adding a trait, for example, gets us back to where wasi-common was originally. That style of design has pros and cons: for example it's easier to slot in a custom thing but it's harder to understand as there's yet-another-layer of traits. The wasmtime-wasi crate is already pretty gnarly in terms of trying to understand it given its size so I'd be wary to insert more layers of indirection. (an example of this is that tracing through a wasip1 function call can be pretty daunting today)

Personally I'm an advocate for pushing this complexity into bindgen whenever we can. For example if the complexity here is high enough to warrant a new layer of traits we could try to push the resource management into wit-bindgen instead. That's what we've done with trappable-errors for example (and with).

Again though I'm happy to land this as-is if you'd prefer to defer the trait question into the future, but if it's a guarantee that the trait is coming then it might be worth trying to bottom this out before landing.

Wasmtime GitHub notifications bot (Apr 02 2024 at 21:36):

rylev commented on PR #8282:

This change moves things around enough that I'm a bit worried about merge conflicts if we let it sit for too long. Considering its useful in isolation, I'd personally advocate for landing it now, and being able to discuss the future evolution outside of a PR.

Wasmtime GitHub notifications bot (Apr 02 2024 at 21:42):

alexcrichton submitted PR review:

Good point, sounds reasonable to me!

Wasmtime GitHub notifications bot (Apr 02 2024 at 22:26):

alexcrichton merged PR #8282.

Wasmtime GitHub notifications bot (Jul 17 2024 at 12:46):

badeend commented on PR #8282:

@rylev You introduced a TODO in this PR:

pub fn set_keep_alive_count(&self, value: u32) -> SocketResult<()> {
    // TODO(rylev): do we need to check and clamp the value?
    let view = &*self.as_std_view()?;
    Ok(network::util::set_tcp_keepcnt(view, value)?)
}

The implementation of network::util::set_tcp_keepcnt already clamps it:

const MIN_CNT: u32 = 1;
// Cap it at Linux' maximum, which appears to have the lowest limit across our supported platforms.
const MAX_CNT: u32 = i8::MAX as u32;

sockopt::set_tcp_keepcnt(sockfd, value.clamp(MIN_CNT, MAX_CNT))

Does this resolve your question?

Wasmtime GitHub notifications bot (Jul 17 2024 at 13:32):

rylev commented on PR #8282:

Hmmm it does indeed, I wonder why I didn't see the clamping in the util function...

Wasmtime GitHub notifications bot (Jul 17 2024 at 18:36):

badeend commented on PR #8282:

Ok, no worries. I'll remove it.

Last updated: Nov 22 2024 at 16:03 UTC