Skip to content

Fix issues when artifacts are being downloaded concurrently#755

Merged
LexManos merged 3 commits intoMinecraftForge:FG_4.0from
shartte:fix-697
May 9, 2021
Merged

Fix issues when artifacts are being downloaded concurrently#755
LexManos merged 3 commits intoMinecraftForge:FG_4.0from
shartte:fix-697

Conversation

@shartte
Copy link

@shartte shartte commented Mar 11, 2021

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
Copy link
Member

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
Copy link
Author

shartte commented Mar 26, 2021

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
Copy link
Author

shartte commented Mar 27, 2021

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)

@LexManos LexManos merged commit 0c2ea04 into MinecraftForge:FG_4.0 May 9, 2021
@shartte shartte deleted the fix-697 branch May 9, 2021 17:41
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.

Potential deobf Concurrency Issue

2 participants