search

bmwill / memory-socket

An in-memory socket abstraction for Rust
Apache License 2.0
8 stars 3 forks source link

Remove mutable references for accept and incoming #1

Closed dusty-phillips closed 4 years ago

dusty-phillips commented 4 years ago

The structs being emulated (TcpStream et al) are not &mut for the listening and accept methods, so it seems reasonable that memory-socket shouldn't be either. Since Flume channels don't require mutable references, it was easy to take them out. Tests pass; is there any reason not to do this?

dusty-phillips commented 4 years ago

Ah, tests are not passing when --features=async. I'll see if I can fix it.

bmwill commented 4 years ago

Yeah, unfortunately the way that the async APIs are you're essentially required to have a &mut reference. Though the sync API doesn't have this same issue (as this PR highlights). The question then is: should the sync/async APIs differ with mutability w.r.t. listening for new incoming connections? I think I could be convinced since the stdlib TcpListeners API uses shared references and I think its important to be able to drop in a MemorySocket and MemoryListener into places where the stdlib types are currently being used. Thoughts?

dusty-phillips commented 4 years ago

Yeah, I think it makes sense for the sync APIs to be drop in for the std::net ones.

I note that tokio's Listener::accept() method requires an &mut, but async_std's does not (somehow). So it's not possible to make memory-socket a drop-in replacement simultaneously for both of them anyway. Ah, the joys of a split ecosystem.

bmwill commented 4 years ago

If flume::Receiver::recv_async could be made to not need a &mut then we might be able to change the async api as well. In practice you should only have 1 Receiver (since it doesn't impl Clone) so hopefully it doesn't cause too many issues.

Anyway, if you want to tweak your PR to only change the mutability of the sync api we can go ahead and get that merged at least.

dusty-phillips commented 4 years ago

Should be good to go now.

bmwill commented 4 years ago

Thanks! I'll publish a new release shortly