Fix: race-conditions in GUI updates when downloading HTTP files

[TrueBrain]

Motivation / Problem

Fixes #11636.

HTTP downloads, both on WIndows and Linux, are processed in a thread. This thread calls HTTPCallback object with updates, but these are executed from that download-thread. However, that code assumes it is called from the OpenTTD Game thread, and updates things on the GUI. This turns out to be a bit of a mistake, and causes all kinds of (small) race-conditions.

Description

Put a bit of glue between the HTTPCallback and the HTTP downloaders, called HTTPThreadSafeCallback. This Thread-Safe callback puts the ReceiveData and Failure information on a queue, which is dequeue from the OpenTTD Game thread. That way, thread-safety is restored again, and in result, the Thread Analyzer no longer finds any mistakes.

Sadly, due to how the current code is designed, this gives the need for a bit of mutex magic, to ensure things can be added and removed in moments that are safe to do so. The main issue here is that a callback can cause a new HTTP request to be created, which makes it a tiny bit more messy than I would like.

One fundamental change required here, is that OnReceiveData becomes the owner of data. As otherwise the HTTP downloader would already release the buffer before it could have been processed. On Windows, we already allocated a buffer ourselves. On Linux, this requires allocating another (temporary) buffer, and copying the content. A tiny bit of a slow-down, but as HTTP happens rarely, not something anyone should notice.

Limitations

There is another problem here, that I didn't dare to address. NetworkContent's OnReceiveData is .. special. It really became hard to read and hard to understand. In an ideal world, we would rewrite that piece of code, making that OnReceiveData thread-safe. That would be much more efficient. But, that also increases the chances of introduces new bugs. So I went for this approach to avoid that rabbit hole. The NetworkContent's OnReceiveData is not broken; just .. hard to make thread-safe in its current state.

(for those who don't want to open up the code: it is built under the assumption the first request is always to the BaNaNaS mirror redirect service, which returns the URLs of all the files to download; the 2nd and next requests are then one by one an URL from that list. Instead that this is two HTTP handlers, one simply writing a file, the other handling the list, this is build in a single function with some nasty complexity).

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

• The bug fix is important enough to be backported? (label: 'backport requested')
• This PR touches english.txt or translations? Check the guidelines
• This PR affects the save game format? (label 'savegame upgrade')
• This PR affects the GS/AI API? (label 'needs review: Script API')
  • ai_changelog.hpp, game_changelog.hpp need updating.
  • The compatibility wrappers (compat_*.nut) need updating.
• This PR affects the NewGRF API? (label 'needs review: NewGRF')
  • newgrf_debug_data.h may need updating.
  • PR must be added to API tracker

[rubidium42]

free accepts nullptr to reduce these kinds of special cases needing to be written.

Suggested change

	if (data != nullptr) {
	free(data);
	}
	free(data);

[TrueBrain]

Old habit. We once had a platform where that caused a crash. It didn't follow POSIX all that well :P

[JGRennison]

Using std::unique_ptr<char[]> instead of const char * in the signature of OnReceiveData and in class HTTPThreadSafeCallback may remove the need to have these manual calls to free at all.

[TrueBrain]

Yeah, did consider that, but the WinHTTP backend needs a char * pointer to fill; so that gets a bit tricky, especially as WinHTTP wants it in a bit annoying way. So the only way I saw to do a unique_ptr was to allocate yet another buffer, and copy it over. Which felt a bit meh at that point.

[JGRennison]

You don't have to construct the new []char and the unique_ptr which becomes responsible for it at the same time.
You could do: char *buffer = size == 0 ? nullptr : new char[size]; in WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE.
Then construct your std::unique_ptr<char[]>(static_cast<char *>(info)) in WINHTTP_CALLBACK_STATUS_READ_COMPLETE.

[TrueBrain]

I think I did this correct; at least, it works as expected :P