Skip to content

Fixes various problems revealed by Windows CI runs#702

Merged
SylvainCorlay merged 23 commits intojupyter-xeus:mainfrom
Klaim:various-fixes
Mar 27, 2026
Merged

Fixes various problems revealed by Windows CI runs#702
SylvainCorlay merged 23 commits intojupyter-xeus:mainfrom
Klaim:various-fixes

Conversation

@Klaim
Copy link
Copy Markdown
Contributor

@Klaim Klaim commented Mar 26, 2026

This PR fixes various issues which are either specific to Windows (python path issue) or timing issues randomly revealed by Windows tests runs:

  1. the python debugger object was not destroyed while the GIL was acquired, leading to UB which was more visible on Windows as random access violations (segfaults);
  2. variations in xpython process startup timing, more pronounced on Windows, lead to random tests failing because of zmq's heartbeat check failing and throwing an exception, leading to the test executable crash;
  3. std::system was used in tests to launch the xpython project and using & to run it in parallel and continue, but this only works with bash and on Windows the & is not interpreted the same way by cmd, resulting to tests waiting for xpython to complete on Windows and ultimately leading to timeouts;
  4. the python executable path deduction algorithm was incorrect when I tried locally on Windows (it was looking for the exe in the myenv/Library/ directory instead of myenv/) - note however that I couldnt reproduce that specific problem on the CI which uses explicit env variable to specify this path, it's only visible when there is no such specification this is not considered here as the issue is actually the user not specifying or wrongly specifying PYTHONHOME;
  5. issue 4 or any failed handling of script execution (zmq absorbing uncaught exceptions in that case) could result in zombie xpython running without me noticing while re-running the test, which lead to other errors.

Issues 2, 3, 5 are fixed in this PR by using boost.process (only as a dependency of the test executable) to

  • properly launch xpython subprocess in a cross-platform way and in parallel,
  • read xpython's I/O to wait for it to be ready after launch before proceeding to any testing,
  • and automatically terminate that sub-process once done with testing (through RAII).

Issue 5 is also fixed in this PR by tracking the stilll-running xpython sub-processes until after main() is finished and if some are still running (usually because of errors in xpython) will terminate the remaining sub-process. This works with aborts (resulting from uncaught exceptions or not) but not with quick-exits.

Note that only 4 is specific to Windows, the others could theorically happen on other platforms (speed/timing and UB differences) and the fixes reflect that.

Most of the changes here were first experimented and confirmed working through #685, #686, and experimented with the incoming async changes in #697.

Comment thread src/xpaths.cpp Outdated
Comment thread src/xdebugger.cpp Outdated
{
// release/destroy the debugger python object while GIL is acquired
pybind11::gil_scoped_acquire gil_lock;
auto local_debug = std::move(m_pydebugger);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use the macro for acquiring the GIL and calling decref on the object?

Copy link
Copy Markdown
Contributor Author

@Klaim Klaim Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked for such macro but couldnt find it in pybind11's doc, if you have the name at hand I can try it.

Meanwhile I tried to use ref-decrement instead of moving-locally-and-destroy as it does seem to make more sense. It seems to work as expected locally at least.

Comment thread src/xinterpreter.cpp Outdated
Comment thread src/xinterpreter.cpp Outdated
Copy link
Copy Markdown
Member

@SylvainCorlay SylvainCorlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Comment thread src/xpaths.cpp
@SylvainCorlay SylvainCorlay merged commit b121f84 into jupyter-xeus:main Mar 27, 2026
15 checks passed
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.

3 participants