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 squash] Get rid of the last remaining sync. HTTP requests on the main thread #14649

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

grorp
Copy link
Member

@grorp grorp commented May 13, 2024

Building upon the work in #13551 and #14412, this PR aims to make sure that there will never be ANRs related to HTTP requests again.

  • Commit 1 moves the last remaining HTTP request from the main thread to core.handle_async. To reduce the amount of changes to existing code and to avoid callback hell, it uses coroutines.

    The HTTP request in question is the one that fetches a dependency list from CDB before attempting to install a package. While it's usually very fast, it can take long if you have a bad internet connection or if it times out.

  • Commit 2 removes the ability to execute synchronous HTTP requests on the main thread from httpfetch.cpp. This doesn't break backwards compatibility because fetch_sync is only available in the mainmenu. Since any synchronous HTTP request on the main thread is a bug, we should avoid introducing one again in the future.

To do

This PR is a Ready for Review.

  • Investigate whether more code can be removed from httpfetch.cpp.
    There is probably more to remove, but I fear that I don't know the code well enough to avoid accidentally breaking things.

How to test

Verify that Minetest doesn't freeze for a short while anymore after you press the install button on a package.

Try to install lots of different packages from CDB. Verify that dependencies are resolved correctly and that packages are installed correctly.

end
if package.raw_deps then
return package.raw_deps
local function make_callback_coroutine(fn, callback)
Copy link
Member

Choose a reason for hiding this comment

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

Might be interesting to add a coroutine http api

Copy link
Member Author

@grorp grorp May 13, 2024

Choose a reason for hiding this comment

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

Implemented in 066e07c to give the idea a try.

Differences:

  • core.parse_json runs on the main thread again. Previously, this PR moved it into core.handle_async.
  • The difference between the master branch and this PR is smaller than before.
  • The overall amount of code is still exactly the same.
  • Is this approach more ergonomic? I don't know.

What do you think?

EDIT: I've come to the conclusion that I don't like it, also because I noticed that the old version can be made even shorter.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I just don't understand this resumer-pattern, but would await (like here) be a suitable replacement?
Just a suggestion ofc.

Copy link
Member Author

Choose a reason for hiding this comment

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

When using your await function, the problem I have is that the final callback (the one passed into contentdb.has_hard_deps / contentdb.resolve_dependencies by the API user) is not called. To make this work, the value returned by coroutine.resume here

coroutine.resume(co, ...)

would somehow have to get back to the final callback, which brings me back to my "resumer" pattern.

This is my first time working with coroutines. There might of course be a better approach that I haven't seen.

@grorp grorp force-pushed the no-sync-http-main-thread branch 4 times, most recently from b13197b to d89cd5b Compare May 14, 2024 13:32
@grorp grorp added this to the 5.9.0 milestone May 21, 2024
grorp added 2 commits May 29, 2024 16:18
Any sync. HTTP request on the main thread is a bug, don't allow introducing one again.
@grorp grorp force-pushed the no-sync-http-main-thread branch from d89cd5b to 6245073 Compare May 29, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants