search

MinecraftForge / ForgeGradle

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

Fix issues when artifacts are being downloaded concurrently #755

Closed shartte closed 3 years ago

shartte commented 3 years ago

Fixes #697: Concurrent attempts at downloading artifacts will be synchronized to avoid corrupted files on disk

To actually reproduce this issue, the cache needs to be cleaned first (the one in %USERPROFILE%.gradle). In addition, usually multiple dependencies are needed, but since it's a race condition it's non-deterministic when it happens.

The output that indicates the function was called concurrently and this fix actually prevented potential data corruption looks like this:

Downloading net.minecraft:mappings_official:1.16.5@zip
Waiting for download of net.minecraft:mappings_official:1.16.5@zip on other thread
Downloading net.minecraftforge:installertools:1.1.10:fatjar
Waiting for download of net.minecraftforge:installertools:1.1.10:fatjar on other thread

For our builds on AE2 this mostly happened on the CI/CD since it starts with a clean cache everytime. On top of that this only happened for us when we had at least two fg.deobf dependencies since those semeed to be processed concurrently.

LexManos commented 3 years ago

This looks okay, except for the download key. If your concern is that it is overwriting the two files at once. It'll do it again if the context of the call is not the exact same. So that key should probably just be the artifact/path.

shartte commented 3 years ago

Yes, that is true, I did it this way so it could be placed at the very top, making it easier to follow whats going on.

Looking at this with some distance, Path based would be best, but would make it necessary to move this logic into the download subfunctions (since they seem to determine the path). I could however use the Artifact, since that's the key also used for CACHE. Not perfect, but probably better.

Thoughts?

shartte commented 3 years ago

I took another hard look at this and decided to just comment on why the entire parameter list is used as a composite key: Sadly there's no guarantee that for any given artifact X, the function is only called with the same tuple of (changing, generated, gradle, manual), and I want to avoid skipping certain lookups (i.e. the gradle one) just because it happened concurrently with another type of lookup that had no result.

It'd probably require greater cleanup to this class to avoid that problem entirely (i.e. ensure different sets of lookup types are not used for the same artifact)