diff --git a/python/mujoco/renderer_test.py b/python/mujoco/renderer_test.py index bb5f62f1..055a9476 100644 --- a/python/mujoco/renderer_test.py +++ b/python/mujoco/renderer_test.py @@ -14,6 +14,9 @@ # ============================================================================== """Tests for the MuJoCo renderer.""" +import gc +import sys + from absl.testing import absltest from absl.testing import parameterized import mujoco @@ -157,6 +160,47 @@ class MuJoCoRendererTest(parameterized.TestCase): with self.assertRaises(ValueError): 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 = """ + + + + + + +""" + 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__': absltest.main() diff --git a/python/mujoco/rendering/classic/renderer.py b/python/mujoco/rendering/classic/renderer.py index e2069db8..0e472e58 100644 --- a/python/mujoco/rendering/classic/renderer.py +++ b/python/mujoco/rendering/classic/renderer.py @@ -49,6 +49,11 @@ class Renderer: ValueError: If `camera_id` is outside the valid range, or if `width` or `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_height = model.vis.global_.offheight if width > buffer_width: @@ -80,9 +85,8 @@ the clause: # Create render contexts. # 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: - self._gl_context = gl_context.GLContext(width, height) + self._gl_context = gl_context.GLContext(width, height) # type: ignore if self._gl_context: self._gl_context.make_current() self._mjr_context = mujoco.MjrContext(model, font_scale.value)