Skip to content

IPython/core/history.py: Handle RuntimeError on Thread.start - #14318

Merged
Carreau merged 3 commits into
ipython:mainfrom
passagemath:pyodide-fix
Feb 6, 2024
Merged

IPython/core/history.py: Handle RuntimeError on Thread.start#14318
Carreau merged 3 commits into
ipython:mainfrom
passagemath:pyodide-fix

Conversation

@mkoeppe

@mkoeppe mkoeppe commented Feb 1, 2024

Copy link
Copy Markdown
Collaborator

On the emscripten (Pyodide), HistorySavingThread.start raises RuntimeError.
We handle this exception here.

It also reveals a misplaced atexit call, which we also fix here.

@mkoeppe
mkoeppe marked this pull request as ready for review February 1, 2024 18:44
@mkoeppe mkoeppe mentioned this pull request Feb 1, 2024
2 tasks
Comment thread IPython/core/history.py

@only_when_enabled
def run(self):
atexit.register(self.stop)

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.

Hum, interesting. I haven't looked at this code in while, and not too familiar with threading.

Would it also be more correct to atexit.unregister(self.stop) in def stop(self) itself ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Seems reasonable, done in 047ab6e

Comment thread IPython/core/history.py
"Failed to start history saving thread. History will not be saved.",
exc_info=True,
)
self.hist_file = ":memory:"

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.

+1. Should we also have a condition that check if we are im emscripten to be explicit ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

At the moment, I have to say that I don't have a clear picture of what codepaths in IPython have a chance to be useful within emscripten (cf. pyodide/pyodide#4452 (comment)). So I'd be reluctant to propose specific checks for the emscripten platform in places that do not already have platform-specific code (in contrast to #14316, thanks for merging that!).

@Carreau Carreau added this to the 8.22 milestone Feb 3, 2024
@Carreau
Carreau merged commit 0a3cf8f into ipython:main Feb 6, 2024
@Carreau

Carreau commented Feb 6, 2024

Copy link
Copy Markdown
Member

Thanks !

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.

2 participants