Skip to content

[release-1.12] Revert "Scheduler: Use a "scheduler" task for thread sleep (#57544)" #58764

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: backports-release-1.12
Choose a base branch
from

Conversation

kpamnany
Copy link
Member

This reverts commit 640c4e1.

See discussion.

Cc: @KristofferC to confirm that this PR is done correctly (to release-1.12).

Cc: @Keno to confirm whether reverting this on 1.12 alone is fine or if you want this reverted on master.

@Keno
Copy link
Member

Keno commented Jun 17, 2025

I'm ok with 1.12 only

@kpamnany
Copy link
Member Author

From the discussion in #58689:

Do we not properly try/catch/restart that task?

I was thinking of adding exactly this as an alternative to backing this out entirely. This patch can be considered a bugfix as it did close 2 issues.

We don't, but even if we did that, that would just result in us swallowing all InterruptExceptions.

The scheduler task should only be running if there are no other runnable tasks. If we add a try/catch to the scheduler task and swallow InterruptExceptions, then that seems to me like an adequate stopgap solution until there is action on #52291.

Thoughts @Keno and @vtjnash?

@gbaraldi
Copy link
Member

Can you open a PR that does the try catch behaviour? Just so that we can see how it behaves?

@kpamnany
Copy link
Member Author

Can you open a PR that does the try catch behaviour? Just so that we can see how it behaves?

See #58765.

@nsajko nsajko added the revert This reverts a previously merged PR. label Jun 17, 2025
@IanButterworth IanButterworth changed the title Revert "Scheduler: Use a "scheduler" task for thread sleep (#57544)" [release-1.12] Revert "Scheduler: Use a "scheduler" task for thread sleep (#57544)" Jun 17, 2025
@Keno
Copy link
Member

Keno commented Jun 17, 2025

I would pretty strongly advocate for a straight revert here. The problem with underdefined behavior is that people will have written their existing code to whatever happened to work, so even small changes are likely to break people's workflows. I don't see that the motivation to let the task be GC'ed slightly earlier is sufficiently strong to force this through and we don't have enough time left to really let people try a new behavior to assess impcat.

@fonsp
Copy link
Member

fonsp commented Jun 24, 2025

From the Julia IDE perspective, I would really like to see interrupt testing in Julia CI to prevent issues like this in the future. Interrupt is a crucial feature for an interactive programming language. If interrupt works reliably, (new) users will feel much more comfortable with running and trying code, which speeds up the developer process.

I wrote a testable code snippet in #58798 (comment) that you can use.

Malt.jl also has many interrupt tests (about Distributed and Malt), and one reason for splitting this out of Pluto.jl was to get reliable coverage from PkgEval. So it's frustrating for me to see that PkgEval was not used in #57544 which would have caught this issue.

@nsajko
Copy link
Contributor

nsajko commented Jun 24, 2025

I would really like to see interrupt testing in Julia CI to prevent issues like this in the future.

I guess you're right, but this tangent seems slightly off-topic here and might be forgotten. You probably want to open a new issue or PR, regarding regression testing specifically.

@@ -547,12 +547,14 @@ end
fetch(r)
end

let addr = Sockets.InetAddr(ip"192.0.2.5", 4444)
let addr = Sockets.InetAddr(ip"127.0.0.1", 4444)
Copy link
Member

Choose a reason for hiding this comment

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

unrelated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Sorry, will ensure that only the PR changes are reverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, that was a PR change! 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, I fixed this test.

@KristofferC KristofferC changed the base branch from release-1.12 to backports-release-1.12 June 24, 2025 17:07
@KristofferC
Copy link
Member

Changed PR to be against backport branch.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
revert This reverts a previously merged PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants