Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

make @sync lexically scoped and merge @schedule with @async #27164

Merged
merged 1 commit into from
May 23, 2018

Conversation

JeffBezanson
Copy link
Sponsor Member

With the upcoming threading runtime we will likely be adding more parallel constructs. Unfortunately, we have a few too many similar things (@async, @schedule, @threads, @spawn, ...). Confusion between @async and @schedule is particularly common and unnecessary; for example some code in the Sockets stdlib uses @async where it should have used @schedule.

This change improves the situation by removing @schedule. The key change to make this possible is to make @sync lexically scoped. That way, if an @async appears on its own it just spawns a task and nothing else happens. But if it's lexically surrounded by a @sync, then that sync waits for all the async stuff inside.

It looks to me like basically all uses of @sync in the stdlib work with lexical scope. If they don't, it's pretty hard to find that out since dynamic scope is so hard to reason about, giving another reason to make this change :)

@JeffBezanson JeffBezanson added breaking This change will break code parallelism Parallel or distributed computation labels May 18, 2018
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

My one comment would be that it is easier to deprecate @async since that is the behaviour this changes. But in general I don't mind much and I think that @async is the better name.

@JeffBezanson JeffBezanson changed the title RFC: make @sync lexically scoped and merge @schedule with @async make @sync lexically scoped and merge @schedule with @async May 21, 2018
@JeffBezanson JeffBezanson added the needs news A NEWS entry is required for this change label May 21, 2018
@JeffBezanson JeffBezanson removed the needs news A NEWS entry is required for this change label May 22, 2018
NEWS.md Outdated
@@ -472,6 +472,11 @@ This section lists changes that do not have deprecation warnings.
* `mv`,`cp`, `touch`, `mkdir`, `mkpath` now return the path that was created/modified
rather than `nothing` ([#27071]).

* `@sync` now waits only for *lexically* enclosed (i.e. visible directly in the source
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth mentioning the deprecation of @schedule, no?

@JeffBezanson JeffBezanson merged commit baa1437 into master May 23, 2018
@JeffBezanson JeffBezanson deleted the jb/async branch May 23, 2018 14:29
Liozou pushed a commit to Liozou/julia that referenced this pull request May 24, 2018
@samoconnor
Copy link
Contributor

I'm a little worried by the removal of @schedule.

Some piece of code might call the new @async with the intention of starting a long-running background task. That will work fine until someone else adds a second short-lived @async operation in the same function and wraps the whole block in @sync.
This is not too hard to debug if the original long-running task never terminates, because the program will just hang and it shouldn't be too hard to spot the cause. However, if the original long-running task usually runs for a few seconds then terminates the result will be a hard to track down performance problem.

Another potential pitfall is a small function that starts out with a @sync block and a couple of @async tasks. Over time the function grows, and eventually someone decides to refactor the guts into seperate functions (or a future automated refactoring tool does this). A seemingly simple refactoring step "move this chunk of code into a function" will result in code that might work fine some of the time but might fail in subtle ways because the @async task is no longer waited for.

I can sympathise with @JeffBezanson's desire to remove unneeded confusion between @async and @schedule, however it might be better to do something like:

  • leave @schedule as it was, but maybe only document it under e.g. "Tasks - Advanced",
  • make it an error to use @async without an enclosing @sync block.

Another possibility would be to remove @sync and have

tasks = []
push!(tasks, @async begin
    ...
end)
push!(tasks, @async begin
    ...
end)
wait(tasks)

@iamed2
Copy link
Contributor

iamed2 commented Jun 5, 2018

I think that's an important concern.

Bringing back schedule but keeping the new behaviour for async seems like a good idea to me.

vtjnash added a commit that referenced this pull request Sep 28, 2022
This is the test from #27164.

The test was checking whether sleep(1) (which is started first) or
sleep(0.05) (which is nominally shorter) returned first. Switch the
order so that the short sleep should always end first.

Fixes #46360
vtjnash added a commit that referenced this pull request Sep 29, 2022
This is the test from #27164.

The test was checking whether sleep(1) (which is started first) or
sleep(0.05) (which is nominally shorter) returned first. Use an Event
instead so that they are explicitly ordered.

Fixes #46360
vtjnash added a commit that referenced this pull request Sep 29, 2022
This is the test from #27164.

The test was checking whether sleep(1) (which is started first) or
sleep(0.05) (which is nominally shorter) returned first. Use an Event
instead so that they are explicitly ordered.

Fixes #46360
Keno pushed a commit that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code parallelism Parallel or distributed computation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants