Skip to content
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

gio::SocketService: Add a Stream implementation for the incoming connections #1064

Closed
sdroege opened this issue Mar 22, 2023 · 5 comments
Closed
Labels
enhancement New feature or request gio

Comments

@sdroege
Copy link
Member

sdroege commented Mar 22, 2023

See title

@sdroege sdroege added enhancement New feature or request gio labels Mar 22, 2023
@sdroege sdroege changed the title gio::SocketListener: Add a Stream implementation for the incoming connections gio::SocketService: Add a Stream implementation for the incoming connections Mar 22, 2023
@jf2048
Copy link
Member

jf2048 commented Apr 30, 2023

I think this would be similar to #431

@carlosmn
Copy link
Contributor

carlosmn commented Jul 6, 2024

Would it have to be a Stream? I've been looking into it but it looks like the regular way you'd do this with tokio or async-std has the shape of

while let Ok(stream) = listener.accept().await {
    accept_connection(stream)
}

which seems more like a one-shot bridge between the signal and the async world. We can of course still have it as

let incoming = service.incoming();
while let Some(stream) = incoming.next() {
   accept_connection(stream)
}

but it might be beneficial for it to look more like the other async world. Or it might be more beneficial to look more like other glib stuff?

The underlying SocketListener has a whole host of accept* methods which would make this a bit harder to differentiate (including accept_async).

@carlosmn
Copy link
Contributor

carlosmn commented Jul 7, 2024

Actually async-std does provide an incoming() method so either way there is prior art.

But while working on this it looks like a bigger issue is the inversion of control between the callback method and the Stream. On a Stream, you choose when to accept the next connection, whereas with callbacks, the SocketService decides when to call.

The naïve implementation of passing along connections from the connect_incoming into a channel that the caller can listen on works well enough for most cases, but the SocketService is going to choose how quickly new connections are added to that list, and they've already been accepted.

A more complex solution here I think would wait for a poll_next and then subscribe to connect_incoming for one call and pass back a connection whenever it gets it. Looking through the SockerService code, I'm not sure if this would stop it from accepting connections we're not ready for, but even if it does, it seems wasteful how much setup and teardown we're doing.

Given that "all" it does is call SocketListener's accept_async and emit the signal when that completes, it might make more sense for this Stream to exist as part of SocketListener so that on poll_next it calls SocketListener::accept_async so we can call the waker when that completes. If the inheritance rules work out, that would also make it available on SocketService(and ThreadedSocketService though it would not very useful if we do want threads).

All this to say, it feels like SocketService is the callback version that makes SocketListener less awkward in single-threaded code and an async Stream would be the solution in the async world, so building it on top of it is going to be super awkward.

@carlosmn
Copy link
Contributor

carlosmn commented Jul 9, 2024

Given #1454 is this done now? Or is there more?

@sdroege
Copy link
Member Author

sdroege commented Jul 9, 2024

I'd say this is done now. Thanks :)

@sdroege sdroege closed this as completed Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gio
Projects
None yet
Development

No branches or pull requests

3 participants