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

Raise error if EventEmitter used with async callback #312

Merged
merged 5 commits into from
Nov 22, 2024

Conversation

bcherry
Copy link
Contributor

@bcherry bcherry commented Nov 21, 2024

Some reasonable feedback arrived in community that it's confusing that you can't use async handlers for events in this SDK.

I looked at the complexity of supporting async handlers and it seems like the largest barrier would be how to await them? Either the .emit() method would need to become async and await them directly, which sounds wrong as you wouldn't expect .emit() to block on long-running applications, or you'd need to create tasks to run them asynchronously but then you have the question of where to collect the tasks and how to cancel them or cleanup.

I think it's reasonable to pass this onto the end-developer to collect their own async tasks but we can make it easier to discover this and harder to write subtle bugs. Raising an error with an explanation and workaround suggestion if you pass an async callback seems like a good start.

Copy link
Member

@nbsp nbsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there no way to do this within python's typing instead of throwing our own error? otherwise lg

@nbsp
Copy link
Member

nbsp commented Nov 21, 2024

i think next steps would be to detect whether it's a coro and wrap it ourselves, because i agree the method of doing it right now is kind of cumbersome

@bcherry
Copy link
Contributor Author

bcherry commented Nov 21, 2024

is there no way to do this within python's typing instead of throwing our own error? otherwise lg

AFAIK python doesn't have any runtime type checking facilities. We do provide the necessary type hints but I think it requires some special configuration to get your local IDE to pull them through correctly.

i think next steps would be to detect whether it's a coro and wrap it ourselves, because i agree the method of doing it right now is kind of cumbersome

Yes it's easy to accept coroutines and then create a task to run them in the emit method, I'm just not sure how to think about who owns these tasks and is responsible for cancelling or draining them at shutdown so its a larger effort. We have a smaller version of this problem in RPC, but there at least it made sense to have the room cancel open callbacks upon disconnect because we know what the callbacks are for. Here they are potentially very open-ended and a lot of different kinds of classes extend EventEmitter in livekit-rtc and agents

assert len(calls) == 2
assert calls[0] == (1, 2, 3)
assert calls[1] == (1, 2, 3, 4, 5)
assert calls == [(1, 2, 3, 4, 5), (1, 2)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was flaky because callbacks are stored in a set and thus aren't explicitly ordered, and this check relied on a particular callback order to work.

I broke the test in two (one for regular args and one for varargs) and fixed the flakiness

@bcherry bcherry merged commit cebfe80 into main Nov 22, 2024
12 checks passed
@bcherry bcherry deleted the bcherry/warn-async branch November 22, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants