Fix AttributeError in Renderer.__del__ on partial construction.
If Renderer.__init__ raises before the rendering contexts are assigned (e.g. width > offwidth raises ValueError, or MjrContext construction fails), __del__ calls close() which accesses self._gl_context and self._mjr_context unconditionally, raising AttributeError. This masks the real __init__ failure with a noisy "Exception ignored in..." message during garbage collection. Pre-initialize both attributes to None at the top of __init__ so that close() is safe on a partially-constructed instance. Add a regression test that captures sys.unraisablehook and asserts that __del__ raises nothing when __init__ fails via the width > offwidth path. Fixes #3213.
This commit is contained in:
@@ -14,6 +14,9 @@
|
|||||||
# ==============================================================================
|
# ==============================================================================
|
||||||
"""Tests for the MuJoCo renderer."""
|
"""Tests for the MuJoCo renderer."""
|
||||||
|
|
||||||
|
import gc
|
||||||
|
import sys
|
||||||
|
|
||||||
from absl.testing import absltest
|
from absl.testing import absltest
|
||||||
from absl.testing import parameterized
|
from absl.testing import parameterized
|
||||||
import mujoco
|
import mujoco
|
||||||
@@ -157,6 +160,47 @@ class MuJoCoRendererTest(parameterized.TestCase):
|
|||||||
with self.assertRaises(ValueError):
|
with self.assertRaises(ValueError):
|
||||||
renderer.render(out=np.zeros((*failing_render_size, 3), np.uint8))
|
renderer.render(out=np.zeros((*failing_render_size, 3), np.uint8))
|
||||||
|
|
||||||
|
def test_renderer_del_safe_when_init_fails_early(self):
|
||||||
|
"""Regression test for #3213.
|
||||||
|
|
||||||
|
Renderer.__del__ must be safe on a partially-constructed instance when
|
||||||
|
__init__ raises before the rendering contexts are assigned. Previously,
|
||||||
|
AttributeError from __del__ masked the real __init__ exception.
|
||||||
|
"""
|
||||||
|
xml = """
|
||||||
|
<mujoco>
|
||||||
|
<visual>
|
||||||
|
<global offwidth="50" offheight="50"/>
|
||||||
|
</visual>
|
||||||
|
<worldbody/>
|
||||||
|
</mujoco>
|
||||||
|
"""
|
||||||
|
model = mujoco.MjModel.from_xml_string(xml)
|
||||||
|
|
||||||
|
# Capture any exception raised from __del__ on the partially-constructed
|
||||||
|
# Renderer. Without the fix, __del__ raises AttributeError, which is
|
||||||
|
# funneled through sys.unraisablehook.
|
||||||
|
unraisable = []
|
||||||
|
old_hook = sys.unraisablehook
|
||||||
|
sys.unraisablehook = lambda args: unraisable.append(args)
|
||||||
|
try:
|
||||||
|
# width > offwidth raises ValueError in __init__ before
|
||||||
|
# self._gl_context is assigned.
|
||||||
|
with self.assertRaises(ValueError):
|
||||||
|
mujoco.Renderer(model, height=50, width=200)
|
||||||
|
gc.collect()
|
||||||
|
finally:
|
||||||
|
sys.unraisablehook = old_hook
|
||||||
|
|
||||||
|
self.assertEqual(
|
||||||
|
[u.exc_type.__name__ for u in unraisable],
|
||||||
|
[],
|
||||||
|
msg=(
|
||||||
|
'Renderer.__del__ raised on a partially-constructed instance; '
|
||||||
|
'see #3213.'
|
||||||
|
),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
if __name__ == '__main__':
|
if __name__ == '__main__':
|
||||||
absltest.main()
|
absltest.main()
|
||||||
|
|||||||
@@ -49,6 +49,11 @@ class Renderer:
|
|||||||
ValueError: If `camera_id` is outside the valid range, or if `width` or
|
ValueError: If `camera_id` is outside the valid range, or if `width` or
|
||||||
`height` exceed the dimensions of MuJoCo's offscreen framebuffer.
|
`height` exceed the dimensions of MuJoCo's offscreen framebuffer.
|
||||||
"""
|
"""
|
||||||
|
# Pre-initialize context attributes so __del__ -> close() is safe even if
|
||||||
|
# __init__ raises below before they are assigned. See #3213.
|
||||||
|
self._gl_context = None # type: ignore
|
||||||
|
self._mjr_context = None
|
||||||
|
|
||||||
buffer_width = model.vis.global_.offwidth
|
buffer_width = model.vis.global_.offwidth
|
||||||
buffer_height = model.vis.global_.offheight
|
buffer_height = model.vis.global_.offheight
|
||||||
if width > buffer_width:
|
if width > buffer_width:
|
||||||
@@ -80,9 +85,8 @@ the clause:
|
|||||||
|
|
||||||
# Create render contexts.
|
# Create render contexts.
|
||||||
# TODO(nimrod): Figure out why pytype doesn't like gl_context.GLContext
|
# TODO(nimrod): Figure out why pytype doesn't like gl_context.GLContext
|
||||||
self._gl_context = None # type: ignore
|
|
||||||
if gl_context.GLContext is not None:
|
if gl_context.GLContext is not None:
|
||||||
self._gl_context = gl_context.GLContext(width, height)
|
self._gl_context = gl_context.GLContext(width, height) # type: ignore
|
||||||
if self._gl_context:
|
if self._gl_context:
|
||||||
self._gl_context.make_current()
|
self._gl_context.make_current()
|
||||||
self._mjr_context = mujoco.MjrContext(model, font_scale.value)
|
self._mjr_context = mujoco.MjrContext(model, font_scale.value)
|
||||||
|
|||||||
Reference in New Issue
Block a user