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

No way to implement external events due to blocking recv_event #109

Open
tuxmark5 opened this issue May 11, 2017 · 7 comments
Open

No way to implement external events due to blocking recv_event #109

tuxmark5 opened this issue May 11, 2017 · 7 comments
Labels

Comments

@tuxmark5
Copy link

Right now the only way to poll events from a Connection is to use recv_event, which is a blocking operation. As a result, it's impossible right now to implement a bot that listens on external events and produces Discord commands based on those events.

To fix this, there should be either a non-blocking alternative to recv_event, or a Future/Stream interface for streaming the events.

@anna-is-cute
Copy link

recv_event being the only method in which to receive events also makes it incredibly difficult to implement a cleanly-stopping bot that uses Connection.

For example, to cleanly stop the main event loop, you have to create a new thread and move Connection inside. Inside the thread, recv_event will block and you use a channel to send events out of the thread, where the main event loop, using select!, can process events and also get a shutdown signal to cleanly break out of the main loop when necessary (Ctrl-C, supervisord, etc.).

This is fine, but when you need access to Connection, such as when you want your bot to connect to voice, it gets hairier. If you make an Arc<Mutex<Connection>>, you can almost make it work, but recv_event blocking indefinitely means that other threads waiting to get a lock on Connection must wait until another event is received and the Mutex becomes unlocked.

I'd love to see a different way to receive events that enables this flow to work.

@SpaceManiac
Copy link
Owner

Clean shutdowns triggered by external events are probably the thorniest situation caused by the current blocking recv_event structure.

There are a couple of ways to handle other event sources. If all you need is to trigger Discord API calls based on events from other sources, run them in their own thread and use an Arc<Discord> to share the Discord struct between them, allowing the other threads to make API calls. If you need other sources to know current state, pass around an Arc<RwLock<State>>. If you really want to have your own event loop be the main event loop, put Connection in a thread and feed the result of recv_event down an mpsc::channel forever, as discussed.

Connecting to voice based on an external event is another point of difficulty. I intend to analyze all this at some point and figure out what can be done. The main obstacle is building on top of the underlying websocket and TLS wrappers which aren't immediately amenable to non-blocking or asynchronous I/O. I also think tokio is too young and complicates both library and client code too thoroughly to be worth implementing just yet, though it has a very promising future.

@jD91mZM2
Copy link
Contributor

jD91mZM2 commented Jun 5, 2017

If possible, the simplest solution could be to not take a mutable reference in recv_event. Sadly, I suspect an upstream repository has it that way?

@tinaun
Copy link
Contributor

tinaun commented Jun 17, 2017

the latest websocket has a .set_nonblocking() method on streams and clients. because sslstreams cannot be split anymore (its unsafe), i was thinking about moving the entire client into the keepalive thread, handling all heartbeat and reconnecting logic there, and adding a second mpsc channel to send events back to the connection object, which could be read in both blocking and nonblocking ways from the connection object.

any thoughts?

i do agree that async would be nice, but its still pretty early and awkward right now, and i feel like its important to keep having sync libs too.

@jD91mZM2
Copy link
Contributor

I think anything that actually makes this possible seems fantastic.

@SpaceManiac
Copy link
Owner

If you can make it work while keeping close to the current interface, I would happily accept that PR.

@domenicquirl
Copy link

Hi everyone! Has there been any progress on this since the issue was last updated? I've run into the same scenario as @ascclemens describes in their final paragraph regarding the blocking call to recv_event getting in the way of an audio thread that also needs the Connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants