Skip to content

Fix test_print_to_correct_cell_from_child_thread - #1312

Merged
minrk merged 2 commits into
ipython:mainfrom
davidbrochart:fix-test
Dec 20, 2024
Merged

Fix test_print_to_correct_cell_from_child_thread#1312
minrk merged 2 commits into
ipython:mainfrom
davidbrochart:fix-test

Conversation

@davidbrochart

Copy link
Copy Markdown
Collaborator

The parent thread has to live for the duration of the child thread, otherwise the parent thread exits before the child thread and disappears from the active threads, causing it to be removed from thread_to_parent and eventually leading to messages not being routed to the right cell:

thread_to_parent_header=stream._thread_to_parent_header
foridentityinlist(thread_to_parent_header.keys()):
ifidentitynotinactive_threads:
try:
delthread_to_parent_header[identity]
exceptKeyError:
pass
thread_to_parent=stream._thread_to_parent
foridentityinlist(thread_to_parent.keys()):
ifidentitynotinactive_threads:
try:
delthread_to_parent[identity]
exceptKeyError:
pass

@krassowski Could you confirm? I saw that while working on #1291.

Comment threadtests/test_kernel.py Outdated
def parent_target():
sleep({interval})
Thread(target=child_target).start()
sleep({interval * iterations})

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.

Does it also work if in addition to this sleep you keep the sleep({interval}) above? I wonder if that was here for a reason. In other words, I think that adding the sleep at the end is fine, I am less confident about the function of the sleep that was removed (as this can change some edge case related to execution order).

Further, should we save a reference to the thread in a local variable before calling sleep()?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

In 41b31c5 I added back the first sleep, and replaced the other sleep with a thread.join(), since it's essentially the same.

@minrk

Copy link
Copy Markdown
Member

I think also related to this is #1289 . Most of the time, in my experience, routing output to the cell that happened to spawn the thread not the right thing to do, and results in quite a bit of mysteriously lost and out-of-order output.

@krassowski

krassowski commented Dec 20, 2024

Copy link
Copy Markdown
Member

Thanks for linking #1289, I missed it when it was opened (despite the mention!). I see your point. I think that usually routing to the spawning cell is the right thing to do, but will happily agree that there should be a way to opt out for the scenarios that you present.

@davidbrochart

Copy link
Copy Markdown
CollaboratorAuthor

Failure is unrelated:

FAILED tests/test_subshells.py::test_run_concurrently_sequence[True-are_subshells0] - AssertionError: assert 'error' == 'ok'

@krassowskikrassowski left a comment

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.

LGTM!

@minrk
minrk merged commit 6f2bc84 into ipython:mainDec 20, 2024
@davidbrochart
davidbrochart deleted the fix-test branch December 20, 2024 19:42
ianthomas23 pushed a commit to ianthomas23/ipykernel that referenced this pull request Jul 14, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@davidbrochart@minrk@krassowski