Skip to content

Fix AttributeError in egl.GLContext.__del__ on partial construction - #3497

Open
shoemoney wants to merge 1 commit into
google-deepmind:mainfrom
shoemoney:fix-egl-glcontext-del-attributeerror
Open

Fix AttributeError in egl.GLContext.__del__ on partial construction#3497
shoemoney wants to merge 1 commit into
google-deepmind:mainfrom
shoemoney:fix-egl-glcontext-del-attributeerror

Conversation

@shoemoney

Copy link
Copy Markdown

Summary

  • Pre-initializes self._context = None as the first statement of GLContext.__init__ in python/mujoco/egl/__init__.py, so free() (called from __del__) is safe on a partially-constructed instance.
  • Adds a regression test asserting that free()/__del__ do not raise AttributeError on a GLContext that never ran __init__.

Why

On a headless host whose EGL driver lacks the PLATFORM_DEVICE extension, create_initialized_egl_device_display() returns EGL_NO_DISPLAY and GLContext.__init__ raises ImportError at line 97-101, before self._context is ever assigned (it's first set at line 114). When the partially-constructed object is later garbage collected, __del__ calls free(), which unconditionally reads self._context:

def free(self):
  """Frees resources associated with this context."""
  global EGL_DISPLAY
  if self._context:   # AttributeError here if __init__ never got this far
    ...

This raises AttributeError: 'GLContext' object has no attribute '_context' from __del__, surfaced via sys.unraisablehook as a noisy Exception ignored in: <function GLContext.__del__ ...> traceback -- which masks the real, actionable ImportError that told the user EGL is unavailable. Reported in #991.

This is the same bug class fixed for Renderer.__init__/renderer.py in #3225 (issue #3213): pre-initialize the guarded attribute to None before anything that can raise, so __del__ is always safe regardless of how far __init__ got.

The fix is a single line:

def __init__(self, max_width, max_height):
  self._context = None
  del max_width, max_height  # unused
  ...

Test

hasattr(mujoco, 'GLContext')-style tests (used by render_test.py/renderer_test.py) are skipped in this repo's own CI, which runs the Python test job with MUJOCO_GL=disable. Reliably forcing GLContext.__init__ itself to fail the way a real headless host without PLATFORM_DEVICE support would additionally requires an EGL driver that's present but specifically lacks that extension -- not a precondition a test environment can create on demand.

So python/mujoco/egl_gl_context_test.py instead patches create_initialized_egl_device_display (via unittest.mock.patch.object) to return EGL.EGL_NO_DISPLAY, exactly as it would on such a host, and drives a real GLContext(640, 480) call through its real __init__ code path. The test asserts __init__ still raises ImportError (the real, actionable error), then garbage-collects the partially-constructed object under sys.unraisablehook and asserts nothing was raised from __del__. It skips only when the mujoco.egl module itself can't be imported (no libEGL/libOpenGL on the host -- an environment precondition orthogonal to this bug, and the same reason this repo's own CI runs with MUJOCO_GL=disable).

Scope

This PR fixes only the missing-attribute-on-__del__ bug. It does not touch the unrelated EGL_DISPLAY-goes-stale-after-atexit-termination bug that #2510 (open) fixes in the same file's free()/make_current() -- that PR adds EGL_DISPLAY and to the if self._context: check and swaps atexit.register(EGL.eglTerminate, ...) for a wrapper that nulls EGL_DISPLAY after termination; it does not pre-initialize self._context, so it does not fix this bug, and this PR's one-line change does not touch either of the lines #2510 touches.

Validation

Built MuJoCo from source (CMake + Ninja, Release, no IPO/LTO) and the Python bindings against it (Python 3.12), then ran pytest -v --pyargs mujoco with MUJOCO_GL=disable -- matching this repo's own CI invocation (.github/workflows/build.yml, "Test Python bindings" step) -- on both clean main and this branch:

  • Clean main (baseline): 380 passed, 5 failed, 14 skipped.
  • This branch: 380 passed, 5 failed, 15 skipped.

The 5 failures are identical on both and are all plugin-loading failures unrelated to this change (my local build didn't install/copy the plugin shared libraries the way CI's copy_plugins_posix step does):
bindings_test.py::MuJoCoBindingsTest::test_copy_mjdata_with_plugin, test_deepcopy_mjdata_with_plugin, test_load_plugin, specs_test.py::SpecsTest::test_delete_unused_plugin, test_plugin. None of these touch python/mujoco/egl/.

The one extra skip on this branch is the new egl_gl_context_test.py, which skips because this machine (macOS) has no libEGL/libOpenGL for mujoco.egl to import -- same reason the existing render_test.py/renderer_test.py GL tests are skipped here and in this repo's own CI (MUJOCO_GL=disable).

I additionally verified the new test's logic outside of pytest by stubbing the OpenGL.EGL import chain in-process so mujoco.egl becomes importable on a machine without a real EGL driver, then running the test body directly against both the pre-fix and post-fix egl/__init__.py: it fails with the exact AttributeError: 'GLContext' object has no attribute '_context' from __del__ on pre-fix code, and passes on post-fix code.

If GLContext.__init__ raises before self._context is assigned -- e.g.
because the EGL driver lacks PLATFORM_DEVICE support and
create_initialized_egl_device_display() returns EGL_NO_DISPLAY --
__del__ later calls free(), which unconditionally reads self._context
and raises AttributeError. That masks the real, actionable ImportError
raised from __init__. See google-deepmind#991.

Pre-initialize self._context to None as the first statement of
__init__, mirroring the fix already applied to Renderer in google-deepmind#3225.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant