I'm catching up with latest episodes and want to point to couple of issues with asset code.
If somebody will remove hha file during iteration in Win32OpenNextFile function (or FindNextFileA will fail there), then for next file Win32OpenNextFile function will return NULL. But code that uses result of this function doesn't expect that and everything will crash when dereferencing NULL pointer in here:
Code in LoadAssetWork work queue function has race condition. Because multiple threads will be using same data passed to it (platform_file_handle) the errors won't be processed correctly. If ReadDataFromFile function fails (by setting NoError to false) then next call to PlatformNoFileErrors function potentially won't see error and will process error status from the result of different call to ReadDataFromFile function.
That's why stdio file API read function returns result instead of storing it somewhere :)
If somebody will remove hha file during iteration in Win32OpenNextFile function (or FindNextFileA will fail there), then for next file Win32OpenNextFile function will return NULL. But code that uses result of this function doesn't expect that and everything will crash when dereferencing NULL pointer in here:
1 | #define PlatformNoFileErrors(Handle) ((Handle)->NoErrors) |
Code in LoadAssetWork work queue function has race condition. Because multiple threads will be using same data passed to it (platform_file_handle) the errors won't be processed correctly. If ReadDataFromFile function fails (by setting NoError to false) then next call to PlatformNoFileErrors function potentially won't see error and will process error status from the result of different call to ReadDataFromFile function.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 | internal PLATFORM_WORK_QUEUE_CALLBACK(LoadAssetWork)
{
load_asset_work *Work = (load_asset_work *)Data;
Platform.ReadDataFromFile(Work->Handle, Work->Offset, Work->Size, Work->Destination);
CompletePreviousWritesBeforeFutureWrites;
// TODO(casey): Should we actually fill in bogus data here and set to final state anyway?
if(PlatformNoFileErrors(Work->Handle)) // <====== This depends on result of ReadDataFromFile call above
{
Work->Slot->State = Work->FinalState;
}
EndTaskWithMemory(Work->Task);
|
That's why stdio file API read function returns result instead of storing it somewhere :)
mmozeiko
If somebody will remove hha file during iteration in Win32OpenNextFile function (or FindNextFileA will fail there), then for next file Win32OpenNextFile function will return NULL.
That is definitely a mistake - please remind me to fix it on stream if you remembe :) I will try to remember as well! Because this is actually not just a mistake for HHA file removal, I think, it's that I had intended it to always return a file handle that was just invalid rather than no file handle at all. But I guess since it requires storage (unfortunately) that's not strictly possible :/ I will have to rethink that part a little.
Code in LoadAssetWork work queue function has race condition.I don't think that's true, is it? Because the error condition is only one-direction. You can never go from an error state to a no-error state. Once a stream has an error it is considered shut down, and will never go back to a no-error state by some subsequent function. But I will double-check this if you remind me!
- Casey
Edited by Casey Muratori
on
Ok, yeah I guess currently race condition won't give any big issues. Except with minor issue - it possibly won't last valid asset just before the error (it will load, but will think error happened). One possible easy fix is to make ReadDataFromFile to return false in case of error additionally to setting NoError boolean. Then this place can use return value, not the global state.
Yes, that would be fine, although I don't really know that it's necessary. If the asset loading fails, it fails, and that's pretty much the end of the game at that point because the assets have to be there in order for it to work...
- Casey
- Casey