First attempt at multithreading

[glowredman]

Attempts to move certain tasks off of the main thread:

• Fetching all versions
• Indexing client and server jars
• Getting assets from Mojang's servers or cached jars

Unlike before, the last task will be executed as soon as possible (versions have been fetched, jars are indexed and no other assets are currently being processed).

I have next to no experience with multithreading, please review carefully!

[Nikolay-Sitnikov]

Also, are there any timing limitations on when the assets have to be loaded by? I could imagine a scenario where a user with a fast computer but slow Internet tries to use the assets before TX-Loader finishes downloading them.

Two options would be either using awaitShutdown on the thread pool or CompletableFuture.allOf() on the download tasks to block the main thread until all the assets are downloaded if they aren't downloaded by the time they should be.

[glowredman]

Currently, the only timing restriction is that no assets can be loaded before TXLoaderCore.injectData() has been called (which is hardly a restriction). Mods using this (I don't know if there are any, but still) are expected to add their assets as early as possible. I'm a bit hesitant to put any further restrictions on it because the use-case could be arbitrary. A possible solution could be to add a new method to AssetBuilder like this:

public Future<Void> fetch() {
    return RemoteHandler.fetchAsset(this.asset);
}

and modify RemoteHandler.fetchAsset() to return the Future. This way, mods could handle any blocking themselves, if necessary.
This wouldn't solve the executor never being shut down though. How would this be?

[Nikolay-Sitnikov]

SynchronousQueue will block the main thread once the thread capacity on the executor is reached. If you want net requests to queue up, use a LinkedBlockingQueue

[Nikolay-Sitnikov]

If you're going to synchronize on ASSETS everywhere, you don't have to wrap it in a synchronizedList.

[glowredman]

I thought I still had to do it, given its javadoc:

It is imperative that the user manually synchronize on the returned list when iterating over it:

 List list = Collections.synchronizedList(new ArrayList());
     ...
 synchronized (list) {
     Iterator i = list.iterator(); // Must be in synchronized block
     while (i.hasNext())
         foo(i.next());
 }

Failure to follow this advice may result in non-deterministic behavior.

https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedList-java.util.List-

[Nikolay-Sitnikov]

"When iterating over it" is the key phrase. synchronizedList essentially just wraps every call to the list in a synchronized block. Therefore, the list's changes are guaranteed to be synchronized whenever anyone reads or writes it.

The one hole is that the iterator doesn't lock the list, since there is no way for synchronizedList to tell when your code is done using the iterator, and thus no way to know when it is safe to unlock the list. For this reason, .iterator() specifically is not synchronized, and can lead to non-deterministic behavior.

[Nikolay-Sitnikov]

What should we do if the player is offline? I think it would be reasonable to fall back to an on-disk cache for all downloads, and only report issues if the resource is both missing from the cache and cannot be downloaded. (See issue 1 of report.)

[Nikolay-Sitnikov]

Same here. We should cache not just the jars, but the version information too. (We could make a single method, say loadFromCacheOrWeb, which checks the caches first and downloads from the web otherwise.)

[glowredman]

That's probably reasonable but I think we should make the current feature set work first

[Nikolay-Sitnikov]

I don't like that we make VERSIONS a global constant, even though it isn't usable until versionsStage is complete. If access to VERSIONS is not performance-critical, we should make it accessible only through versionsStage to make sure there can't be any code that accesses it before it is initialized.

[Nikolay-Sitnikov]

You should probably throw an error if versionsStage == null. Silently ignoring the request to fetch the asset is entirely unacceptable.

(You could also instead initialize versionsStage to just a simple new CompletableFuture<>() in the static initializer, and then complete the future manually (using CompletableFuture#complete) after receiving the necessary info in the coremod initializer. This would ensure that versionsStage will never be null, and any requests, no matter how early, can get queued up.)

[Nikolay-Sitnikov]

You cannot use a CompletableFuture as a key to a map! It is not the result of the request, it is just an object specifying a request. You cannot hash it, since when you create it, it doesn't even have a value yet!

[glowredman]

A fundamental problem I haven't been able to solve yet (and I don't even know if it's possible without major restructuring) is that when when scheduling a task which needs to extract from a jar, we need to immediately decide what future to chain it to, but we don't know yet if the JAR is present or if it needs to be downloaded.

One way to solve this would to have the indexing be done synchronously so the question what to chain on would not be ambiguous (is the jar present? -> extract, otherwise download first). But this would be a step away from full concurrency.

[github-actions]

#26

[Eldrinn-Elantey]

Review focused on concurrency correctness per the PR author's request. Findings posted inline.

[Eldrinn-Elantey]

Follow-up on the earlier SynchronousQueue vs LinkedBlockingQueue discussion: switching to LinkedBlockingQueue avoids the main-thread-blocking problem, but it introduces a new one. ThreadPoolExecutor only spawns threads beyond corePoolSize when the queue rejects a task, and an unbounded LinkedBlockingQueue never rejects. With corePoolSize=0, both EXECUTOR_IO and EXECUTOR_NET will settle at exactly one worker thread; maximumPoolSize (512 / 32) never comes into play. Tasks queue up and run one at a time instead of concurrently.

To actually get concurrency with a bounded thread count, set corePoolSize == maximumPoolSize and call allowCoreThreadTimeOut(true) so idle threads still die after the keep-alive.

[Eldrinn-Elantey]

This blocking call is removed with nothing replacing it. Previously getResourceDomains() waited for RemoteHandler.getAssets() before scanning this.dir, guaranteeing downloaded assets existed before Minecraft enumerated resource domains. Now fetchAsset() calls are fire-and-forget: MinecraftHook.insertPacks() and this class never join()/get() on the futures it returns. If the game asks for a domain before its background download finishes, the domain simply won't be present, and nothing re-triggers a rescan once the download completes.

This needs an explicit decision: either block here on the futures scoped to this.dir's assets, or fire a resource-reload once pending fetches for this pack complete.

[Eldrinn-Elantey]

CACHED_CLIENT_JARS/CACHED_SERVER_JARS memoize the future per version via computeIfAbsent in RemoteHandler.fetchAsset(). If a fetch fails or resolves to null (e.g. transient network error while downloading client.jar), that failed future stays cached for the rest of the session; there's no retry path for that version. The previous implementation only populated the cache on success, so a transient failure would simply retry on the next lookup (e.g. F3+T). Worth checking for a failed/null result and evicting the entry so a later attempt can retry.

[Eldrinn-Elantey]

thenCombineAsync requires both input stages to complete before the combiner runs, including getDetails(version), which performs an HTTP GET for the version manifest. That request always fires, even though its result (versionDetails) is discarded a few lines down whenever JarHandler.index(client, version) already found a locally cached jar. This means every jar lookup now requires network connectivity, even for versions fully available on disk, breaking offline-from-cache operation and wasting a request per lookup. Checking JarHandler.index() first and short-circuiting before composing with getDetails() would restore the old cache-first behavior.

[Eldrinn-Elantey]

getModContainerClass()/injectData() no longer special-case FMLLaunchHandler.side().isServer(). Previously dedicated servers returned early after ServerLangHelper.load() and skipped RemoteHandler.getVersions()/JarHandler.indexJars()/ConfigHandler.load()/moveRLAssets() entirely. Now that whole pipeline runs unconditionally, so headless servers reach out to Mojang's version manifest and download/index client+server jars, which they never did before. ServerLangHelper.load() still runs (now via TXLoaderModContainer.serverStarting), so the lang-loading behavior is preserved, but the added network/jar work on dedicated servers isn't mentioned in the PR description. Worth confirming this is intentional.

[Eldrinn-Elantey]

CompletableFuture.runAsync(RemoteHandler::fetchVersions, EXECUTOR_NET) in TXLoaderCore.injectData() discards the returned future. Inside fetchVersions(), only downloadManifest() is wrapped in try/catch (this line); if anything after it throws (e.g. manifest.versions.forEach hitting unexpected null data), the exception completes the discarded future exceptionally with no log line, and versionsStage (plus everything chained off it: cacheStage, all asset fetches) is left permanently incomplete with no diagnostic trail. Worth adding .exceptionally(...) at the injectData() call site for logging.