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

fix(source): HLS would not work if used directly. #262

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

Erk-
Copy link
Member

@Erk- Erk- commented Oct 29, 2024

This changes HlsRequest to return true in
Compose::should_create_async.

This would otherwise cause it to be spawned on a thread without a
executor causing it to panic.

This would only cause issues if used directly since we otherwise use
Compose::create in a context where we have access to the executor.

Closes #257

Open questions:

  • Should the create implementation be removed or have a note that it will panic if called outside of a tokio executor?

@Erk- Erk- mentioned this pull request Oct 29, 2024
@Erk-
Copy link
Member Author

Erk- commented Oct 29, 2024

The fails in the tests seems to be that beta and nightly catches more examples of missing documentation and is not caused by this patch.

Copy link
Member

@FelixMcFelix FelixMcFelix left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for figuring this out. Yeah, CI failures are down to a change in how a lint is applied.

@FelixMcFelix
Copy link
Member

* [ ]  Should the `create` implementation be removed or have a note that it will panic if called outside of a tokio executor?

I think a note indicating the panic would be most useful here. It is still feasible to call, so it doesn't violate the existing API contract but it's better to not surprise users where possible!

This changes HlsRequest to return true in
Compose::should_create_async.

This would otherwise cause it to be spawned on a thread without a
executor causing it to panic.

This would only cause issues if used directly since we otherwise use
Compose::create in a context where we have access to the executor.
@Erk-
Copy link
Member Author

Erk- commented Nov 14, 2024

I added a note about it on the struct

@FelixMcFelix
Copy link
Member

Thanks for the fix; feel free to merge when you're ready (I believe you have access?).

@Erk- Erk- merged commit 312799d into serenity-rs:current Nov 15, 2024
11 checks passed
@Erk- Erk- deleted the fix-hls-directly branch November 15, 2024 04:47
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.

HLS stream would panic
3 participants