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] Avoid use of select(2) #14255

Merged
merged 3 commits into from Jan 17, 2024
Merged

[no squash] Avoid use of select(2) #14255

merged 3 commits into from Jan 17, 2024

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Jan 13, 2024

select() is a fundamentally broken API that shouldn't be used anymore. So this is what we'll do.

This raises the minimum curl version to at least 7.28.0. It was released sometime in 2012.

To do

This PR is Ready for Review.

How to test

  1. try single player
  2. try mainmenu
  3. try this nearly pointless mod that performs HTTP requests:
local http_api = core.request_http_api()
assert(http_api)

local testn = 0
local function dotest()
	if testn >= 6 then
		print("done")
		return
	end
	testn = testn + 1
	local my_n = testn
	print("begin " .. my_n)
	http_api.fetch({
		url = "https://httpbin.org/delay/2",
	}, function(res)
		print("end " .. my_n .. " " .. tostring(res.succeeded))
	end)
	core.after(1 + testn/4, dotest)
end
core.after(3, dotest)
  1. ensure it doesn't hang at shutdown (any more than before)

@sfan5 sfan5 added @ Network Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Jan 13, 2024
@grorp
Copy link
Member

grorp commented Jan 15, 2024

ensure it doesn't hang at shutdown

This doesn't happen to have something to do with #12815, does it? I did a short test and Minetest still hangs if I try to close it while downloading a package from ContentDB.

@sfan5
Copy link
Member Author

sfan5 commented Jan 16, 2024

This doesn't happen to have something to do with #12815, does it? I did a short test and Minetest still hangs if I try to close it while downloading a package from ContentDB.

Not related, no.
We had #12983 for that but IMO it would be better for the mainmenu to use the async httpfetch API. Then this problem would also not exist.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Media loading still works. Looks good.

@sfan5 sfan5 merged commit 5756d62 into minetest:master Jan 17, 2024
13 checks passed
@sfan5 sfan5 deleted the noselect branch January 17, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Network One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants