MinecraftForge / ForgeGradle

Minecraft mod development framework used by Forge and FML for the gradle build system
GNU Lesser General Public License v2.1
517 stars 444 forks source link

[FG5] Fix DownloadAssets race condition #801

Closed shartte closed 3 years ago

shartte commented 3 years ago

Fixes #800.

LexManos commented 3 years ago

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

shartte commented 3 years ago

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

LexManos commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Alright I've changed it. Is this better?