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

Make entry point synchronous and switch to global runtime instead. #996

Merged
merged 3 commits into from
Dec 11, 2022

Conversation

ThomasFrans
Copy link
Contributor

@ThomasFrans ThomasFrans commented Nov 26, 2022

An example to illustrate what I mean in issue #993. This isn't meant to be merged (therefore a draft PR), but more as a way to show the proposed changes in the issue and potentially get input on this approach. Not a lot would have to change. From what I understand about async, this wouldn't have negative performance implications, but I might be wrong about this. I tested this and it works like before.

@hrkfdn
Copy link
Owner

hrkfdn commented Dec 10, 2022

So I did some more code (re-)reading and it looks like it makes sense to proceed with this one, as there's barely any async usage starting from main (as you also pointed out). We can still revert if that changes. Do you agree or do you want to make more adjustments (as this is still a draft PR)?

@ThomasFrans
Copy link
Contributor Author

I think I noticed two more futures::block_on that I missed as this was just a draft. They also need to be switched. I'll quickly do that, or you can change them. Then I think it's good to merge.

@ThomasFrans ThomasFrans marked this pull request as ready for review December 11, 2022 10:39
@ThomasFrans
Copy link
Contributor Author

There are still two futures::future::pending() calls, I think somewhere in the spotify_worker.rs file. I'm still not too familiar with the difference between futures::* stuff and just the Tokio stuff (like futures::executor vs tokio::runtime). I think it's ok, as I tried without credentials which worked fine, and the pending happens in a bit of code that is responsible for fetching the token, so I think it should work.

@hrkfdn
Copy link
Owner

hrkfdn commented Dec 11, 2022

Those should be fine. futures::pending() just returns a never ending async task that is non-blocking (i.e. doesn't block a thread using sleep or similar). These are used as a placeholder for when there is no token task instead of an Option. Thanks for the adjustments, hope I'll be able to review and merge this later today.

@hrkfdn hrkfdn merged commit ccce78a into hrkfdn:main Dec 11, 2022
@ThomasFrans ThomasFrans deleted the sync-entry branch December 12, 2022 10:06
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.

None yet

2 participants