Merge pull request #3225 from devshahofficial:fix/renderer-del-init-failure

PiperOrigin-RevId: 908640282
Change-Id: I4f474423f94ec785598ed9c2d5b74eba3d927697

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 = """
+<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__':
   absltest.main()

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)