Close the Newton viewer before dropping the reference to it - #7590
Draft
fatimaanes wants to merge 1 commit into
Draft
Close the Newton viewer before dropping the reference to it#7590fatimaanes wants to merge 1 commit into
fatimaanes wants to merge 1 commit into
Conversation
NewtonVisualizer gave up its viewer in two places -- close() and the step() handler that permanently disables the viewer after an unrecoverable failure -- by assigning self._viewer = None without calling viewer.close() first. ViewerRTX.close() releases GPU resources in a fixed order: it waits on the in-flight render, drops the retained step results, unbinds the transform AttributeBinding and only then releases the ovrtx.Renderer. Skipping it leaves all four objects to the garbage collector, which guarantees no order. When the collector reaches the Renderer first it tears itself down with the binding and step results still live, warning "Renderer destroyed with 1 active binding(s)" and logging "Leaking step result outputs" once per retained render product. Route both paths through a single _release_viewer() helper that closes the viewer and clears the reference in a finally block, so an unusable viewer is never retained while the teardown failure still reaches the caller. The two callers need different error handling. close() lets the failure propagate -- SimulationContext._update_visualizers() already wraps it -- but runs its own remaining cleanup in a finally block so a failing viewer cannot strand the generated camera prims or leave _is_closed unset. The step() handler contains the failure, because that handler exists so an unusable viewer disables itself instead of aborting training; letting a teardown error escape would replace the original failure and crash the run it was written to save. ViewerBase.close() is a no-op, so the GL, null and file backends are unaffected. The viser and rerun visualizers already close their viewers. The _Viewer double in test_newton_adapter.py gains the close() method that every real viewer inherits from ViewerBase; without it the double no longer models the viewers it stands in for.
github-actions
Bot
added
bug
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.
Description
NewtonVisualizerdropped its viewer without closing it in two paths:close()step()recovery path that disables the viewer after an initialization failureThis left OVRTX resources to be released by the garbage collector in an undefined order. If the renderer was destroyed before its active bindings and retained step results, shutdown could report:
Both paths now use a shared
_release_viewer()helper that callsviewer.close()before clearing the reference.During normal shutdown, teardown failures continue to propagate after the remaining cleanup completes. If viewer cleanup also fails in the
step()recovery path, the error is logged without interrupting training.Type of change
Release backport
developThe same issue is present on
release/3.0.0.Validation
Controlled teardown reproduction
The controlled test reproduced the reported shutdown signature:
Windows — RTX PRO 6000
Ran each of the two reported workloads three times, for six runs per batch:
The OVRTX window failed to initialize on this machine, so every run exercised the
step()recovery path. A pre-existing USD asset-loading crash occurred once in each batch before viewer initialization and is being tracked separately.Linux — L40
The nine new regression tests fail against the original implementation and pass with the fix:
Seven tests requiring the full Isaac Sim runtime were excluded because of a pre-existing collection error.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/CONTRIBUTORS.md