Add auto-download rendering wrapper support for DXVK and D3D9On12#3472
Open
bwiemz wants to merge 2 commits intoFAForever:developfrom
Open
Add auto-download rendering wrapper support for DXVK and D3D9On12#3472bwiemz wants to merge 2 commits intoFAForever:developfrom
bwiemz wants to merge 2 commits intoFAForever:developfrom
Conversation
📝 WalkthroughWalkthroughAdds rendering wrapper support: new RenderingBackend preference and UI, services/tasks to download/cache/extract wrapper DLLs from GitHub, injection/removal around game launch, configuration and i18n entries, tests, and a small .gitignore addition. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as SettingsController
participant Service as RenderingWrapperService
participant GitHub as GitHub API
participant Task as DownloadRenderingWrapperTask
participant FS as File System
participant Launcher as ForgedAllianceLaunchService
User->>UI: Select rendering backend
UI->>Service: ensureWrapperAvailableAsync(backend)
Service->>Service: isWrapperInstalled?
alt already installed
Service-->>UI: done
else needs download
Service->>GitHub: queryLatestRelease(url)
GitHub-->>Service: release metadata
Service->>Service: resolve asset URL
Service->>Task: create & submit download task
Task->>GitHub: download archive
GitHub-->>Task: archive stream
Task->>FS: extract d3d9.dll to cache
FS-->>Task: saved + version.txt
end
User->>Launcher: Start game
Launcher->>Service: injectRenderingWrapper(executeDir)
Service->>FS: copy cached DLLs -> game dir
FS-->>Service: files copied
Launcher->>Launcher: launch process
Launcher->>Service: on process exit -> removeRenderingWrapper()
Service->>FS: delete injected DLLs
FS-->>Service: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Allow users to select Vulkan (DXVK) or DirectX 12 (D3D9On12) as the rendering backend in settings. The client auto-downloads wrapper DLLs from GitHub releases on first use, injects them into the game directory before launch, and cleans up on exit. Falls back gracefully to DirectX 9 if download fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bwiemz
force-pushed
the
feature/#3471-rendering-wrapper-support
branch
from
3403ba0 to
9b0e517
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java (2)
67-94:ensureWrapperAvailableAsyncduplicates the task-setup code fromdownloadWrapper.Lines 79–90 repeat the exact same
queryLatestRelease → resolveAssetUrl → downloadTaskFactory.getObject() → setBackend/setDownloadUrl/setVersion → submitTasksequence already indownloadWrapper(lines 107–116). Extract a shared helper that builds and submits the task, letting the two callers differ only in how they handle the returned future (.join()vs.exceptionally(...)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java` around lines 67 - 94, The code in ensureWrapperAvailableAsync duplicates the task-build/submit sequence found in downloadWrapper; extract a single helper method (e.g., buildAndSubmitDownloadTask) that encapsulates queryLatestRelease(backend) → resolveAssetUrl(backend, release) → downloadTaskFactory.getObject() setup (setting backend, downloadUrl, version) and returns the CompletableFuture from taskService.submitTask(task); replace the duplicated blocks in ensureWrapperAvailableAsync and downloadWrapper to call this helper and let ensureWrapperAvailableAsync attach .exceptionally(...) while downloadWrapper calls .join() on the returned future.
96-100:getWrapperDirectorypath logic is duplicated inDownloadRenderingWrapperTask.
RenderingWrapperService.getWrapperDirectory(lines 96–100) andDownloadRenderingWrapperTask.getWrapperDirectory(lines 141–145) compute identical paths. If the storage layout changes, both must be updated in sync. Consider moving the canonical path computation toRenderingWrapperService(alreadypublic) and having the task delegate to it via constructor injection, or extract a shared constant/utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java` around lines 96 - 100, RenderingWrapperService.getWrapperDirectory currently duplicates path logic found in DownloadRenderingWrapperTask.getWrapperDirectory; refactor so there is a single canonical implementation in RenderingWrapperService and make DownloadRenderingWrapperTask delegate to it (or to a new shared utility) via constructor injection: remove the duplicate logic from DownloadRenderingWrapperTask.getWrapperDirectory, add a RenderingWrapperService (or utility) field to DownloadRenderingWrapperTask and call renderingWrapperService.getWrapperDirectory(backend) wherever needed; ensure constructors are updated to accept the service and tests/usage are adjusted accordingly.src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java (1)
141-145: DuplicategetWrapperDirectory— same comment as flagged inRenderingWrapperService.This private method is byte-for-byte identical to
RenderingWrapperService.getWrapperDirectory(backend)(public). Having the path formula in two places means a future layout change requires two edits. Consider injecting or delegating to the service, or extracting a shared static helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java` around lines 141 - 145, The getWrapperDirectory method in DownloadRenderingWrapperTask duplicates RenderingWrapperService.getWrapperDirectory(backend); remove the private getWrapperDirectory from DownloadRenderingWrapperTask and delegate to the single source of truth by injecting or accessing RenderingWrapperService and calling RenderingWrapperService.getWrapperDirectory(backend) (or extract a shared static helper used by both classes if dependency injection isn't feasible); update DownloadRenderingWrapperTask to use the service/helper and remove the redundant path-construction logic so future layout changes only require one edit.src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java (1)
58-77: Consider verifying injection occurred before cleanup.The assertion
assertFalse(Files.exists(tempDir.resolve("exec/d3d9.dll")))is correct but passes in two distinct scenarios: (1) the DLL was injected and then cleaned up (desired path), and (2) the injection step silently no-oped (e.g.,Files.isDirectory(wrapperDir)returned false). Since both theensureWrapperAvailablemock andgetWrapperDirectorymock are already in place, adding averifymakes the intent unambiguous.💡 Suggested improvement
assertThrows(GameLaunchException.class, () -> instance.launchOfflineGame("test")); +// Verify injection was attempted +verify(renderingWrapperService).getWrapperDirectory(RenderingBackend.DIRECTX_12); // Wrapper is injected, process fails to start, wrapper is cleaned up assertFalse(Files.exists(tempDir.resolve("exec/d3d9.dll")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java` around lines 58 - 77, The test testStartGameOfflineWithWrapperInjection currently only asserts the wrapper DLL is absent after launch which is ambiguous; update the test to explicitly verify that the wrapper injection was attempted by adding Mockito verifications for renderingWrapperService.ensureWrapperAvailable(RenderingBackend.DIRECTX_12) and renderingWrapperService.getWrapperDirectory(RenderingBackend.DIRECTX_12) (or any specific injection method used) before asserting cleanup so the test proves the wrapper was injected (or attempted) and then removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java`:
- Around line 190-223: The injection currently overwrites existing DLLs and
removeRenderingWrapper blindly deletes them; change the injection routine in
ForgedAllianceLaunchService (the block that builds injectedFiles using
wrapperDir, executeDirectory and sourceDll/targetDll) to detect when targetDll
already exists and instead of overwriting either skip injection or move the
existing file to a backup location and record that backup; replace injectedFiles
(Set<Path>) with a structure that records whether each target was newly injected
or backed up (e.g., Map<Path,Optional<Path>> or two sets like injectedFiles and
backedUpFiles) and update removeRenderingWrapper to, for each recorded
targetDll, restore a backed-up file (move backup back to target) if present or
delete the injected file only if there was no original; ensure logging
distinguishes skipped vs backed-up vs injected cases and handle IOExceptions as
currently done.
In
`@src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java`:
- Around line 75-88: In downloadArchive, the URLConnection obtained from
downloadUrl.openConnection() has no connect/read timeouts which can block
indefinitely; set explicit connect and read timeouts on the URLConnection (e.g.
via setConnectTimeout and setReadTimeout) before calling
urlConnection.getInputStream(), using sensible values or a configurable
constant, then proceed with the existing ResourceLocks,
ByteCopier.from(...).to(...).totalBytes(...).listener(this::updateProgress).copy()
flow; keep the timeout values accessible/configurable and reference
downloadArchive and downloadUrl when making the change.
- Line 83: Replace the use of URLConnection.getContentLength() with
getContentLengthLong() when passing the value to ByteCopier.totalBytes(): locate
the call in DownloadRenderingWrapperTask where
.totalBytes(urlConnection.getContentLength()) is used and change it to use
urlConnection.getContentLengthLong() so the long return type matches
ByteCopier.totalBytes(long) and avoids truncation for payloads >2GB.
In
`@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`:
- Around line 102-105: The current isWrapperInstalled(Path) only checks for
d3d9.dll so cached wrappers never get updated; change the check to read the
wrapper's version.txt (written by DownloadRenderingWrapperTask) and compare its
contents to the release tagName you query for the latest wrapper; if the file is
missing or the tag differs, return false so DownloadRenderingWrapperTask will
re-download and then write the new version.txt (ensure
DownloadRenderingWrapperTask writes the exact tagName into version.txt). Use the
unique symbols isWrapperInstalled, DownloadRenderingWrapperTask, version.txt and
tagName to locate and update the logic and the writer consistency.
- Around line 116-127: Both calls block indefinitely: replace the unbounded
CompletableFuture.join() used at taskService.submitTask(task).getFuture().join()
with a bounded wait (e.g., get(timeout, TimeUnit)) and handle TimeoutException
by cancelling the task and surfacing a clear error; and replace the Reactor
.block() in queryLatestRelease(RenderingBackend) (on
defaultWebClient...bodyToMono(GitHubRelease.class).block()) with a timed block
(e.g., block(Duration.ofSeconds(...))) or use .timeout(Duration) on the Mono and
handle the resulting TimeoutException/TimeoutSignal to return or throw a
controlled error. Ensure you update queryLatestRelease to propagate or wrap
timeout failures and clean up/cancel the background task when the future times
out.
- Around line 130-141: In resolveAssetUrl(RenderingBackend backend,
GitHubRelease release) change the current
assets.stream().filter(...).findFirst() selection to use a stricter match on
GitHubAssets.getName() (or explicitly exclude known variant substrings) instead
of just endsWith("." + archiveFormat); for example build and apply a regex that
matches the exact release filename pattern (e.g. ^dxvk-\d+(\.\d+)*\.tar\.gz$ or
a pattern derived from backend) or filter out names containing "-native" /
"steamrt" so the code reliably picks the intended asset and then map to
getBrowserDownloadUrl as before.
In
`@src/test/java/com/faforever/client/fa/rendering/RenderingWrapperServiceTest.java`:
- Around line 88-117: The test testEnsureWrapperAvailableTriggersDownload
currently verifies that DownloadRenderingWrapperTask has setBackend and
setVersion called but misses asserting setDownloadUrl; update the test to also
verify(task).setDownloadUrl(...) using the exact browser download URL created
for the GitHubAssets (the URL passed to asset.setBrowserDownloadUrl /
asset.getBrowserDownloadUrl()) so the test asserts the full task setup contract
for DownloadRenderingWrapperTask and will fail if setDownloadUrl is removed or
passed null.
---
Nitpick comments:
In
`@src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java`:
- Around line 141-145: The getWrapperDirectory method in
DownloadRenderingWrapperTask duplicates
RenderingWrapperService.getWrapperDirectory(backend); remove the private
getWrapperDirectory from DownloadRenderingWrapperTask and delegate to the single
source of truth by injecting or accessing RenderingWrapperService and calling
RenderingWrapperService.getWrapperDirectory(backend) (or extract a shared static
helper used by both classes if dependency injection isn't feasible); update
DownloadRenderingWrapperTask to use the service/helper and remove the redundant
path-construction logic so future layout changes only require one edit.
In
`@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`:
- Around line 67-94: The code in ensureWrapperAvailableAsync duplicates the
task-build/submit sequence found in downloadWrapper; extract a single helper
method (e.g., buildAndSubmitDownloadTask) that encapsulates
queryLatestRelease(backend) → resolveAssetUrl(backend, release) →
downloadTaskFactory.getObject() setup (setting backend, downloadUrl, version)
and returns the CompletableFuture from taskService.submitTask(task); replace the
duplicated blocks in ensureWrapperAvailableAsync and downloadWrapper to call
this helper and let ensureWrapperAvailableAsync attach .exceptionally(...) while
downloadWrapper calls .join() on the returned future.
- Around line 96-100: RenderingWrapperService.getWrapperDirectory currently
duplicates path logic found in DownloadRenderingWrapperTask.getWrapperDirectory;
refactor so there is a single canonical implementation in
RenderingWrapperService and make DownloadRenderingWrapperTask delegate to it (or
to a new shared utility) via constructor injection: remove the duplicate logic
from DownloadRenderingWrapperTask.getWrapperDirectory, add a
RenderingWrapperService (or utility) field to DownloadRenderingWrapperTask and
call renderingWrapperService.getWrapperDirectory(backend) wherever needed;
ensure constructors are updated to accept the service and tests/usage are
adjusted accordingly.
In `@src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java`:
- Around line 58-77: The test testStartGameOfflineWithWrapperInjection currently
only asserts the wrapper DLL is absent after launch which is ambiguous; update
the test to explicitly verify that the wrapper injection was attempted by adding
Mockito verifications for
renderingWrapperService.ensureWrapperAvailable(RenderingBackend.DIRECTX_12) and
renderingWrapperService.getWrapperDirectory(RenderingBackend.DIRECTX_12) (or any
specific injection method used) before asserting cleanup so the test proves the
wrapper was injected (or attempted) and then removed.
src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java
Show resolved
Hide resolved
src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java
Outdated
Show resolved
Hide resolved
Comment on lines
+102
to
+105
| private boolean isWrapperInstalled(Path wrapperDir) { | ||
| return Files.isDirectory(wrapperDir) | ||
| && Files.exists(wrapperDir.resolve("d3d9.dll")); | ||
| } |
There was a problem hiding this comment.
version.txt is written but never read — wrappers are never updated after initial download.
isWrapperInstalled only tests for the existence of d3d9.dll, so once a wrapper is cached it will be reused indefinitely. DownloadRenderingWrapperTask writes a version.txt but nothing reads it. Users will be silently stuck on the version first downloaded, even if a critical DXVK or D3D9On12 update ships.
Consider comparing the cached version against the queried release's tagName before short-circuiting, or at minimum document this as a known limitation.
💡 Sketch of version-aware check
- private boolean isWrapperInstalled(Path wrapperDir) {
- return Files.isDirectory(wrapperDir)
- && Files.exists(wrapperDir.resolve("d3d9.dll"));
- }
+ private boolean isWrapperInstalled(Path wrapperDir, String latestVersion) {
+ if (!Files.isDirectory(wrapperDir) || !Files.exists(wrapperDir.resolve("d3d9.dll"))) {
+ return false;
+ }
+ Path versionFile = wrapperDir.resolve("version.txt");
+ if (!Files.exists(versionFile)) {
+ return false;
+ }
+ try {
+ String cached = Files.readString(versionFile).strip();
+ return cached.equals(latestVersion);
+ } catch (IOException e) {
+ return false;
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`
around lines 102 - 105, The current isWrapperInstalled(Path) only checks for
d3d9.dll so cached wrappers never get updated; change the check to read the
wrapper's version.txt (written by DownloadRenderingWrapperTask) and compare its
contents to the release tagName you query for the latest wrapper; if the file is
missing or the tag differs, return false so DownloadRenderingWrapperTask will
re-download and then write the new version.txt (ensure
DownloadRenderingWrapperTask writes the exact tagName into version.txt). Use the
unique symbols isWrapperInstalled, DownloadRenderingWrapperTask, version.txt and
tagName to locate and update the logic and the writer consistency.
src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java
Show resolved
Hide resolved
src/test/java/com/faforever/client/fa/rendering/RenderingWrapperServiceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java (1)
67-94: No deduplication guard against concurrent download submissions.
ensureWrapperAvailableAsynchas no in-progress check. If the user rapidly toggles the backend selector (or settings are opened while a download is already underway), multipleDownloadRenderingWrapperTaskinstances for the same backend are submitted concurrently, wasting bandwidth and potentially causing file-write races ond3d9.dll/version.txt.Consider tracking an in-flight download per backend (e.g.,
Map<RenderingBackend, CompletableFuture<?>> inProgress) and skipping submission if one is already running.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java` around lines 67 - 94, ensureWrapperAvailableAsync can submit duplicate downloads; add an in-flight guard: introduce a class-level Map<RenderingBackend, CompletableFuture<?>> (or ConcurrentHashMap) named e.g. inProgressDownloads, check if inProgressDownloads contains the backend at the start of ensureWrapperAvailableAsync (after isWrapperInstalled) and return if present, then when creating the DownloadRenderingWrapperTask (via downloadTaskFactory) submit it to taskService and put the returned CompletableFuture into inProgressDownloads; attach completion handlers to that future to remove the backend from inProgressDownloads on both success and failure (and still log exceptions as currently done). Use the unique symbols ensureWrapperAvailableAsync, getWrapperDirectory, isWrapperInstalled, downloadTaskFactory, taskService, and DownloadRenderingWrapperTask to locate the code to change.src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java (1)
59-77:testStartGameOfflineWithWrapperInjectiononly validates cleanup, not injection.The test asserts that
exec/d3d9.dllis absent after the failed launch, but never confirms it was actually copied during the launch. If injection were silently no-oped (e.g., theforEachswallowed anIOException), the assertion would still pass. Consider adding a spy/capture or an intermediateassertTrueto verify the DLL was injected before the process threw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java` around lines 59 - 77, The test only checks cleanup but not that the wrapper was actually injected; modify testStartGameOfflineWithWrapperInjection so you catch the GameLaunchException (or replace assertThrows with a try/catch) around instance.launchOfflineGame("test"), and inside the catch assertTrue(Files.exists(tempDir.resolve("exec/d3d9.dll"))) to verify the DLL was copied during launch before asserting it was removed afterward; use the same tempDir/exec path and preserve the final assertFalse to confirm cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/faforever/client/preferences/ui/SettingsController.java`:
- Around line 498-503: The listener on renderingBackendComboBox (in
initRenderingBackendSelection) calls
renderingWrapperService.ensureWrapperAvailableAsync(newBackend), but
ensureWrapperAvailableAsync currently performs a blocking call
(queryLatestRelease(backend).block()) on the JavaFX thread; change the call site
or the service so the GitHub query runs off the FX thread: either (preferable)
refactor RenderingWrapperService.ensureWrapperAvailableAsync to start the
network call on a background thread / Scheduler (so it never blocks callers), or
wrap the invocation in the listener with an explicit off-FX-thread execution
(e.g., run in a CompletableFuture/Executor) before calling
ensureWrapperAvailableAsync; reference symbols:
renderingBackendComboBox.getSelectionModel().selectedItemProperty listener,
renderingWrapperService.ensureWrapperAvailableAsync(...), and
queryLatestRelease(backend).block().
---
Duplicate comments:
In `@src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java`:
- Around line 190-224: The code currently overwrites existing DLLs via
Files.copy(..., StandardCopyOption.REPLACE_EXISTING) and then deletes all paths
in injectedFiles in removeRenderingWrapper, which can permanently remove user
files; update injectRenderingWrapper logic to detect existing target files
before copying, move any pre-existing target to a safe backup location (or
rename with a .bak + unique suffix) and record a mapping from injected target
Path -> original backup Path (or null if none) instead of only a Set<Path>
(update the field/return type used by removeRenderingWrapper accordingly), and
change removeRenderingWrapper to restore backups when present (move backup back
to original location) or delete only the DLLs that were introduced by the
wrapper, never blindly delete user files; reference the methods/variables
injectRenderingWrapper/injectedFiles, the Files.copy(...) call that uses
StandardCopyOption.REPLACE_EXISTING, and removeRenderingWrapper so reviewers can
find and replace the behavior.
In
`@src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java`:
- Around line 75-89: The downloadArchive method currently opens a URLConnection
with downloadUrl.openConnection() which uses infinite JDK timeouts; set sensible
connect and read timeouts on that URLConnection (or cast to HttpURLConnection)
before calling getInputStream so a stalled server cannot hang downloadArchive
and the download lock; update downloadArchive to call
urlConnection.setConnectTimeout(...) and urlConnection.setReadTimeout(...) with
appropriate constants and ensure the finally block still calls
ResourceLocks.freeDownloadLock().
- Line 83: Replace the int-based content length call with the long-based API: in
DownloadRenderingWrapperTask (around the ByteCopier.totalBytes(...) usage),
change the call that passes urlConnection.getContentLength() to use
urlConnection.getContentLengthLong() so totalBytes() receives a correct long
value and avoids truncation for sizes >2GB or -1 semantics.
In
`@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`:
- Around line 102-105: isWrapperInstalled currently only checks for d3d9.dll so
cached wrappers never update; change isWrapperInstalled(Path wrapperDir) to also
read and validate the wrapper's version by resolving
wrapperDir.resolve("version.txt") and comparing its contents to the
expected/current wrapper version (e.g. a constant or method that returns the
desired version), and treat the wrapper as not installed when the file is
missing, unreadable, or the version does not match so the install/update path
will run; keep the existing d3d9.dll existence check but add this version-file
validation and ensure proper error handling when reading version.txt so
mismatches trigger re-download/reinstall.
- Around line 116-127: Replace the indefinite blocking calls by applying
timeouts: in downloadWrapper (where you currently call
taskService.submitTask(task).getFuture().join()) use a timed wait on the
CompletableFuture (e.g., get with a Duration/TimeUnit or
CompletableFuture.orTimeout/orJoin with a deadline) and propagate or handle
TimeoutException; and in queryLatestRelease (the method that does
defaultWebClient...bodyToMono(...).block()) use Reactor's timed block variant
(Mono.block(Duration)) or add a timeout operator on the Mono before blocking so
the FX thread cannot hang; ensure callers like ensureWrapperAvailableAsync /
SettingsController handle the timeout path and surface an appropriate error
instead of blocking forever.
- Around line 130-141: The current resolveAssetUrl method uses endsWith("." +
archiveFormat) and findFirst(), which can pick the wrong build (e.g., a
"-native" SteamRT variant); change the selection to deterministically pick the
correct asset by filtering and prioritizing: use release.getTagName() (or
backend.getName()/identifier) to match the expected version prefix (e.g.,
asset.getName().startsWith("dxvk-" + release.getTagName()) or
asset.getName().contains(backend.getIdentifier())) and explicitly exclude
variants like "-native" (asset.getName().contains("-native") -> exclude), or
sort candidates so non-native artifacts come first before taking the first;
modify the stream in resolveAssetUrl (referencing resolveAssetUrl,
GitHubAssets::getName, GitHubAssets::getBrowserDownloadUrl, archiveFormat,
release.getTagName()) to apply these stricter filters/prioritization and then
map to the download URL, throwing the same runtime error if none match.
In
`@src/test/java/com/faforever/client/fa/rendering/RenderingWrapperServiceTest.java`:
- Around line 113-116: In testEnsureWrapperAvailableTriggersDownload in
RenderingWrapperServiceTest, add a verification that the download URL is set on
the submitted task: after verify(task).setVersion("v2.5.3") call
verify(task).setDownloadUrl(expectedUrl) (compute expectedUrl the same way
production code does or use the constant/mocked value used in the test setup) so
the test asserts task.setDownloadUrl(...) is invoked when
ensureWrapperAvailable(RenderingBackend.VULKAN_DXVK) triggers a download;
reference the task mock and RenderingWrapperService.ensureWrapperAvailable to
locate where to add this verify.
---
Nitpick comments:
In
`@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`:
- Around line 67-94: ensureWrapperAvailableAsync can submit duplicate downloads;
add an in-flight guard: introduce a class-level Map<RenderingBackend,
CompletableFuture<?>> (or ConcurrentHashMap) named e.g. inProgressDownloads,
check if inProgressDownloads contains the backend at the start of
ensureWrapperAvailableAsync (after isWrapperInstalled) and return if present,
then when creating the DownloadRenderingWrapperTask (via downloadTaskFactory)
submit it to taskService and put the returned CompletableFuture into
inProgressDownloads; attach completion handlers to that future to remove the
backend from inProgressDownloads on both success and failure (and still log
exceptions as currently done). Use the unique symbols
ensureWrapperAvailableAsync, getWrapperDirectory, isWrapperInstalled,
downloadTaskFactory, taskService, and DownloadRenderingWrapperTask to locate the
code to change.
In `@src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java`:
- Around line 59-77: The test only checks cleanup but not that the wrapper was
actually injected; modify testStartGameOfflineWithWrapperInjection so you catch
the GameLaunchException (or replace assertThrows with a try/catch) around
instance.launchOfflineGame("test"), and inside the catch
assertTrue(Files.exists(tempDir.resolve("exec/d3d9.dll"))) to verify the DLL was
copied during launch before asserting it was removed afterward; use the same
tempDir/exec path and preserve the final assertFalse to confirm cleanup.
src/main/java/com/faforever/client/preferences/ui/SettingsController.java
Show resolved
Hide resolved
- Add connect/read timeouts on URLConnection in DownloadRenderingWrapperTask - Use getContentLengthLong() instead of getContentLength() - Deduplicate task setup code into buildAndSubmitDownloadTask helper - Remove duplicate getWrapperDirectory from task, use setter from service - Add version.txt validation to isWrapperInstalled - Add timeouts to Mono.block() and CompletableFuture.get() - Filter out -native asset variants in resolveAssetUrl - Add concurrent download guard with ConcurrentHashMap - Back up and restore existing DLLs instead of overwriting - Run ensureWrapperAvailableAsync off FX thread via CompletableFuture.runAsync - Add setDownloadUrl/setWrapperDirectory verification in tests - Add injection verification in ForgedAllianceLaunchServiceTest Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java (1)
167-176: Injection before try-block means an unexpected RuntimeException skips cleanup.
injectRenderingWrapperis called at line 167 before thetryblock. If an unanticipated unchecked exception occurs between line 167 and line 170 (e.g.,processBuilder.start()throwing a non-IOExceptionerror, or aSecurityException), the injected DLLs won't be cleaned up. Consider moving the injection inside thetryor wrapping the whole block.Proposed fix
log.info("Starting Forged Alliance with command: {} in directory: {}", processBuilder.command(), executeDirectory); Map<Path, Path> injectedFiles = injectRenderingWrapper(executeDirectory); - try { Process process = processBuilder.start(); process.onExit().whenCompleteAsync((p, e) -> removeRenderingWrapper(injectedFiles)); return process; - } catch (IOException exception) { + } catch (Exception exception) { removeRenderingWrapper(injectedFiles); - throw new GameLaunchException("Error launching game process", exception, "game.start.couldNotStart"); + if (exception instanceof IOException ioException) { + throw new GameLaunchException("Error launching game process", ioException, "game.start.couldNotStart"); + } + throw new GameLaunchException("Error launching game process", exception, "game.start.couldNotStart"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java` around lines 167 - 176, injectRenderingWrapper is called before the try block so a RuntimeException thrown before process start can skip removeRenderingWrapper and leak injected files; move the call to injectRenderingWrapper inside the try (or wrap the injection + process start in a try/finally) so that removeRenderingWrapper(injectedFiles) is always invoked on both checked and unchecked exceptions — update the block around processBuilder.start(), injectRenderingWrapper(...), removeRenderingWrapper(...), and the GameLaunchException handling to ensure cleanup even on RuntimeException or SecurityException.src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java (1)
108-112: Path matching is case-sensitive — may miss DLLs with different casing in archives.
matchesEntryPathusesString.equals/endsWith, which is case-sensitive. If a future release ships withD3D9.dllord3d9.DLL, the DLL won't be found and the task will throw. Windows file systems are case-insensitive, so archive tools occasionally produce mixed-case entries.Proposed fix
private static boolean matchesEntryPath(String entryName, String dllPath) { - String normalized = entryName.replace('\\', '/'); - String normalizedDll = dllPath.replace('\\', '/'); + String normalized = entryName.replace('\\', '/').toLowerCase(); + String normalizedDll = dllPath.replace('\\', '/').toLowerCase(); return normalized.equals(normalizedDll) || normalized.endsWith("/" + normalizedDll); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java` around lines 108 - 112, The path comparison in matchesEntryPath is currently case-sensitive and can miss DLLs with different casing; change it to perform a case-insensitive comparison by normalizing slashes and converting both entryName and dllPath to a consistent case (e.g., toLowerCase(Locale.ROOT)) before comparing; then use the same logic (equals or endsWith("/" + dll)) on the lowercased values so entries like "D3D9.dll" or "d3d9.DLL" will match correctly.src/test/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTaskTest.java (1)
139-142:Files.list()stream should be closed — resource leak in test.
Files.list()returns a stream backed by aDirectoryStreamthat must be explicitly closed. Terminal operations likecount()do not close it. Minor since it's test code, but a good habit.Proposed fix
- long tempFiles = Files.list(wrapperDir) - .filter(p -> p.getFileName().toString().startsWith("rendering-wrapper")) - .count(); - assertEquals(0, tempFiles); + try (var files = Files.list(wrapperDir)) { + long tempFiles = files + .filter(p -> p.getFileName().toString().startsWith("rendering-wrapper")) + .count(); + assertEquals(0, tempFiles); + }Same pattern applies to lines 158-161.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTaskTest.java` around lines 139 - 142, Files.list(wrapperDir) returns a Stream that must be closed; wrap the Files.list(...) calls used to compute tempFiles (and the similar call at lines 158-161) in a try-with-resources that captures the Stream<Path> (e.g., try (Stream<Path> s = Files.list(wrapperDir)) { long tempFiles = s.filter(...).count(); }) so the DirectoryStream backing the Stream is closed after the terminal operation; update both occurrences referencing wrapperDir/Files.list to use this pattern.src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java (1)
122-133:buildAndSubmitDownloadTaskperforms a blocking HTTP call — callers must ensure they're off the FX thread.
queryLatestRelease(line 123) calls.block(GITHUB_API_TIMEOUT)on a ReactorMono. This is fine for the current call sites (ensureWrapperAvailablefrom game launch thread,ensureWrapperAvailableAsyncdispatched viaCompletableFuture.runAsync), but this constraint is implicit. A future caller invoking this from the FX thread would freeze the UI for up to 30 seconds. Consider documenting this or extracting the blocking call into a reactive chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java` around lines 122 - 133, buildAndSubmitDownloadTask currently calls queryLatestRelease which uses .block(GITHUB_API_TIMEOUT) and thus performs a blocking HTTP call; avoid freezing the FX thread by moving blocking work off the UI thread or converting the method to a non-blocking reactive chain. Fix by either (A) explicitly running the blocking call on a background executor (e.g., CompletableFuture.supplyAsync wrapping queryLatestRelease) before creating and submitting the DownloadRenderingWrapperTask, or (B) refactor queryLatestRelease to return a Mono and compose the rest of buildAndSubmitDownloadTask as a reactive chain that resolves the release, maps to resolveAssetUrl and DownloadRenderingWrapperTask, then submits the task and converts the final Mono to a CompletableFuture; update callers like ensureWrapperAvailable / ensureWrapperAvailableAsync accordingly or document the blocking behavior if you choose not to refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`:
- Around line 79-93: The TOCTOU bug comes from separate containsKey and put
calls allowing duplicate downloads; modify ensureWrapperAvailableAsync to
atomically insert the download future using
inProgressDownloads.computeIfAbsent(backend, b -> { ... }) so only one
buildAndSubmitDownloadTask(backend) is invoked per backend; inside the mapping
function call buildAndSubmitDownloadTask(backend) and attach the whenComplete
handler there (which removes the entry and logs failures) to ensure the removal
logic still runs; ensure the method returns or uses the returned
CompletableFuture from computeIfAbsent.
---
Duplicate comments:
In
`@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`:
- Around line 105-120: isWrapperInstalled currently only checks presence of
files and non-empty version.txt, so cached wrappers never get refreshed; change
isWrapperInstalled in RenderingWrapperService to read the installedVersion from
version.txt (already done) and compare it against the upstream/latest release
tag (e.g., via a service method like getLatestWrapperVersion() /
fetchLatestWrapperTag() or an injected WrapperReleaseService) and return false
when versions differ so the updater will replace the wrapper; keep the existing
d3d9.dll and versionFile checks, preserve IOException handling/logging, and
ensure the comparison trims/normalizes both versions before comparing.
---
Nitpick comments:
In `@src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java`:
- Around line 167-176: injectRenderingWrapper is called before the try block so
a RuntimeException thrown before process start can skip removeRenderingWrapper
and leak injected files; move the call to injectRenderingWrapper inside the try
(or wrap the injection + process start in a try/finally) so that
removeRenderingWrapper(injectedFiles) is always invoked on both checked and
unchecked exceptions — update the block around processBuilder.start(),
injectRenderingWrapper(...), removeRenderingWrapper(...), and the
GameLaunchException handling to ensure cleanup even on RuntimeException or
SecurityException.
In
`@src/main/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTask.java`:
- Around line 108-112: The path comparison in matchesEntryPath is currently
case-sensitive and can miss DLLs with different casing; change it to perform a
case-insensitive comparison by normalizing slashes and converting both entryName
and dllPath to a consistent case (e.g., toLowerCase(Locale.ROOT)) before
comparing; then use the same logic (equals or endsWith("/" + dll)) on the
lowercased values so entries like "D3D9.dll" or "d3d9.DLL" will match correctly.
In
`@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`:
- Around line 122-133: buildAndSubmitDownloadTask currently calls
queryLatestRelease which uses .block(GITHUB_API_TIMEOUT) and thus performs a
blocking HTTP call; avoid freezing the FX thread by moving blocking work off the
UI thread or converting the method to a non-blocking reactive chain. Fix by
either (A) explicitly running the blocking call on a background executor (e.g.,
CompletableFuture.supplyAsync wrapping queryLatestRelease) before creating and
submitting the DownloadRenderingWrapperTask, or (B) refactor queryLatestRelease
to return a Mono and compose the rest of buildAndSubmitDownloadTask as a
reactive chain that resolves the release, maps to resolveAssetUrl and
DownloadRenderingWrapperTask, then submits the task and converts the final Mono
to a CompletableFuture; update callers like ensureWrapperAvailable /
ensureWrapperAvailableAsync accordingly or document the blocking behavior if you
choose not to refactor.
In
`@src/test/java/com/faforever/client/fa/rendering/DownloadRenderingWrapperTaskTest.java`:
- Around line 139-142: Files.list(wrapperDir) returns a Stream that must be
closed; wrap the Files.list(...) calls used to compute tempFiles (and the
similar call at lines 158-161) in a try-with-resources that captures the
Stream<Path> (e.g., try (Stream<Path> s = Files.list(wrapperDir)) { long
tempFiles = s.filter(...).count(); }) so the DirectoryStream backing the Stream
is closed after the terminal operation; update both occurrences referencing
wrapperDir/Files.list to use this pattern.
Comment on lines
+79
to
+93
| if (inProgressDownloads.containsKey(backend)) { | ||
| log.debug("Download already in progress for {}", backend); | ||
| return; | ||
| } | ||
|
|
||
| log.info("Pre-downloading rendering wrapper for {}", backend); | ||
| try { | ||
| CompletableFuture<?> future = buildAndSubmitDownloadTask(backend); | ||
| inProgressDownloads.put(backend, future); | ||
| future.whenComplete((result, throwable) -> { | ||
| inProgressDownloads.remove(backend); | ||
| if (throwable != null) { | ||
| log.warn("Pre-download of rendering wrapper for {} failed", backend, throwable); | ||
| } | ||
| }); |
There was a problem hiding this comment.
TOCTOU race: containsKey + put is not atomic — duplicate downloads possible.
Two threads calling ensureWrapperAvailableAsync concurrently for the same backend can both pass the containsKey check on line 79, both call buildAndSubmitDownloadTask, and both insert into the map. Use computeIfAbsent to atomically check-and-insert.
Proposed fix
public void ensureWrapperAvailableAsync(RenderingBackend backend) {
if (!backend.requiresDownload()) {
return;
}
Path wrapperDir = getWrapperDirectory(backend);
if (isWrapperInstalled(wrapperDir)) {
return;
}
- if (inProgressDownloads.containsKey(backend)) {
- log.debug("Download already in progress for {}", backend);
- return;
- }
-
log.info("Pre-downloading rendering wrapper for {}", backend);
- try {
- CompletableFuture<?> future = buildAndSubmitDownloadTask(backend);
- inProgressDownloads.put(backend, future);
- future.whenComplete((result, throwable) -> {
- inProgressDownloads.remove(backend);
- if (throwable != null) {
- log.warn("Pre-download of rendering wrapper for {} failed", backend, throwable);
- }
- });
- } catch (Exception e) {
- log.warn("Failed to initiate pre-download of rendering wrapper for {}", backend, e);
- }
+ inProgressDownloads.computeIfAbsent(backend, b -> {
+ try {
+ CompletableFuture<?> future = buildAndSubmitDownloadTask(b);
+ future.whenComplete((result, throwable) -> {
+ inProgressDownloads.remove(b);
+ if (throwable != null) {
+ log.warn("Pre-download of rendering wrapper for {} failed", b, throwable);
+ }
+ });
+ return future;
+ } catch (Exception e) {
+ log.warn("Failed to initiate pre-download of rendering wrapper for {}", b, e);
+ return null;
+ }
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/faforever/client/fa/rendering/RenderingWrapperService.java`
around lines 79 - 93, The TOCTOU bug comes from separate containsKey and put
calls allowing duplicate downloads; modify ensureWrapperAvailableAsync to
atomically insert the download future using
inProgressDownloads.computeIfAbsent(backend, b -> { ... }) so only one
buildAndSubmitDownloadTask(backend) is invoked per backend; inside the mapping
function call buildAndSubmitDownloadTask(backend) and attach the whenComplete
handler there (which removes the entry and logs failures) to ensure the removal
logic still runs; ensure the method returns or uses the returned
CompletableFuture from computeIfAbsent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #3471
Adds automatic downloading and injection of rendering wrapper DLLs (DXVK for Vulkan, D3D9On12 for DirectX 12) so players can use modern graphics APIs with Supreme Commander: Forged Alliance without manual DLL management.
d3d9.dllis copied into the game execution directory at launch and cleaned up on exitFiles changed
ClientProperties.java— newRenderingWrapperconfig section for GitHub release URLsRenderingBackend.java— new enum with archive metadata (format, DLL path in archive)DownloadRenderingWrapperTask.java— new task for downloading and extracting wrapper archives (.tar.gz / .zip)RenderingWrapperService.java— new service orchestrating download, caching, and availability checksForgedAllianceLaunchService.java— integration of wrapper download + DLL injection/cleanup at launchForgedAlliancePrefs.java— newrenderingBackendpreference propertySettingsController.java— rendering backend ComboBox with pre-download triggersettings.fxml— UI layout for rendering backend selectorapplication.yml— GitHub API URLs for DXVK and ForceD3D9On12 releasesmessages.properties— i18n strings for UI labels and download notificationsTest plan
RenderingWrapperServiceTestandForgedAllianceLaunchServiceTest🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Tests