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

Clean closure and task-local-state in jl_finish_task #47829

Closed
wants to merge 2 commits into from

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Dec 7, 2022

Implements the second part of @vtjnash idea in #47405 (comment)

Particularly motivated by @quinnj observation that it is easy to add a return nothing to a task, but the arguments
to the task need to be handled differently. (x-ref #40626 (comment))

@vchuravy vchuravy added the needs pkgeval Tests for all registered packages should be run with this change label Dec 8, 2022
@vchuravy vchuravy requested a review from vtjnash December 8, 2022 15:01
@vchuravy vchuravy added the backport 1.9 Change should be backported to release-1.9 label Dec 8, 2022
@gbaraldi
Copy link
Member

gbaraldi commented Dec 8, 2022

Can someone get a reference to the fields from julia? Or just to the content?

quinnj added a commit to JuliaServices/ConcurrentUtilities.jl that referenced this pull request Dec 8, 2022
As suggested by @vchuravy, and that mirrors the fix proposed in
JuliaLang/julia#47829.
@vchuravy
Copy link
Member Author

vchuravy commented Dec 8, 2022

Can someone get a reference to the fields from julia? Or just to the content?

If you mean &ct.tls. No they are just normal fields in a struct.

quinnj added a commit to JuliaServices/ConcurrentUtilities.jl that referenced this pull request Dec 8, 2022
* Simpler, better wkspawn implementation

As suggested by @vchuravy, and that mirrors the fix proposed in
JuliaLang/julia#47829.

* Fix nightly
Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Can you also add a test that confirms that objects captured by a Task's closure are GC'd when the Task is finished?
Something like

function foo(x::Vector)
    @async length(x)
end
a = []
r = WeakRef(a)
t = foo(a)
a = nothing
wait(t)
GC.gc()
@test r.value === nothing

or somethign like that?

@vchuravy
Copy link
Member Author

vchuravy commented Dec 8, 2022

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@vchuravy
Copy link
Member Author

vchuravy commented Dec 9, 2022

Two packages observe the nothing

AdvancedPS v0.4.3: fail vs. ok
ChannelBuffers v0.2.0: fail vs. ok

@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 9, 2022

AdvancedPS uses LibTask, which violates deep assumptions necessary for fast code, and therefore is undefined behavior and unsupported entirely.

ChannelBuffers is accessing the fields of a closure via reflection, which is an unstable API–though could be easily re-written to use a stable ABI version by making it a struct instead. I suppose though that this is probably valid code however.

@JeffBezanson
Copy link
Sponsor Member

I think the only non-breaking way to do this is to make tls->current_task like a weak reference, such that when that task is finished and current_task is the only reference, some fields inside the task can be cleared.

@vchuravy vchuravy marked this pull request as draft December 21, 2022 15:45
@KristofferC KristofferC mentioned this pull request Dec 28, 2022
14 tasks
@vchuravy vchuravy closed this Jan 1, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jan 2, 2023
@giordano giordano deleted the vc/task_cleanup branch February 14, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants