Skip to content

[FG5] Fix DownloadAssets race condition#801

Merged
LexManos merged 1 commit intoMinecraftForge:FG_5.0from
shartte:fix-asset-download-race-condition
Jun 28, 2021
Merged

[FG5] Fix DownloadAssets race condition#801
LexManos merged 1 commit intoMinecraftForge:FG_5.0from
shartte:fix-asset-download-race-condition

Conversation

@shartte
Copy link

@shartte shartte commented Jun 28, 2021

Fixes #800.

@shartte shartte changed the base branch from FG_4.0 to FG_5.0 June 28, 2021 22:16
@shartte shartte changed the title Fix DownloadAssets race condition [FG5] Fix DownloadAssets race condition Jun 28, 2021
@LexManos
Copy link
Member

This should be done in the first pass, filter out all the keys that are duplicates.

@shartte
Copy link
Author

shartte commented Jun 28, 2021

Which first pass? Stream.distinct wouldn't help here since the keys are unique, the paths aren't.

@LexManos
Copy link
Member

yes, but you can filter it out above the loop instead of inside it.
Or since the key is only used for the pretty debug message, you could not use the key at all.

@shartte
Copy link
Author

shartte commented Jun 28, 2021

Hm yes, i wanted to do this with minimal changes.
Do you see any benefit to filtering it out before? We're iterating over it anyway, and I didn't judge modifying the key list as easier to grasp in my opinion.

p.s.: if you think filtering it out first is better, i'll just do it.

@LexManos
Copy link
Member

LexManos commented Jun 28, 2021

Filtering out first keeps the logical blocks small and simple to understand.
As well as removing the long lasting set that is continuously built/kept for the entire lifecycle of the method.
Also, this method would spam the console every time you run the task because you do it outside of checking if the value actually needs to be downloaded.

You could just do something like:

Set<String> seen;
keys.stream().filter(k -> seen.add(object.get(k).hash))
seen.empty() // Tho not needed, the Jitter will see it go out of use and kill it.

@shartte shartte force-pushed the fix-asset-download-race-condition branch from 938b33f to 3195a0d Compare June 28, 2021 23:21
This happens if two assets use the same remote file, and causes the validation to fail sometimes.
@shartte shartte force-pushed the fix-asset-download-race-condition branch from 3195a0d to 09b6860 Compare June 28, 2021 23:22
@shartte
Copy link
Author

shartte commented Jun 28, 2021

Alright I've changed it. Is this better?

@LexManos LexManos merged commit 72bf288 into MinecraftForge:FG_5.0 Jun 28, 2021
@shartte shartte deleted the fix-asset-download-race-condition branch June 28, 2021 23:52
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.

[FG5] downloadAssets sometimes fails due to a race condition

2 participants