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

Break a Core / Node cycle #5733

Merged
merged 3 commits into from Apr 21, 2018

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented Apr 20, 2018

Problem

Core instances are currently being leaked in cases where Nodes have not completed running, and thus hold a Context in the closure for their Future. The cycle is:

Core -> Graph -> Node -> Context -> Core

Solution

Clear all Node states when we Drop a Scheduler, breaking their cycles with the Core.

Result

Fixes #5732 by ensuring that the Store held via Scheduler -> Core -> Store is dropped. It should also unblock #5611.

@stuhood stuhood changed the title Break an Core / Node cycle Break a Core / Node cycle Apr 20, 2018

@stuhood stuhood added this to the 1.6.x milestone Apr 20, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 20, 2018

An alternate strategy here might be to walk the Graph and "cancel" any Node instances that have not completed... might allow us to avoid the need to panic here?

EDIT: Did this: you can see the two approaches in the first and last commit.

stuhood added some commits Apr 20, 2018

@stuhood stuhood force-pushed the twitter:stuhood/break-core-cycle branch from ac9e874 to 0c3d591 Apr 20, 2018

@dotordogh
Copy link
Contributor

dotordogh left a comment

This is nice! Thank you for doing it! I also ran into this problem this morning and spent some time with @illicitonion trying to debug it.

@@ -166,6 +166,14 @@ impl Scheduler {
}
}

impl Drop for Scheduler {
fn drop(&mut self) {

This comment has been minimized.

@dotordogh

dotordogh Apr 20, 2018

Contributor

From what I can tell, this isn't used yet. Is that intentional?

This comment has been minimized.

@stuhood

stuhood Apr 21, 2018

Member

Drop is called automatically.

This comment has been minimized.

@dotordogh

dotordogh Apr 21, 2018

Contributor

Ahhh I see, thank you!!

@@ -166,6 +166,14 @@ impl Scheduler {
}
}

impl Drop for Scheduler {
fn drop(&mut self) {

This comment has been minimized.

@dotordogh

dotordogh Apr 21, 2018

Contributor

Ahhh I see, thank you!!

@stuhood stuhood merged commit b3f7124 into pantsbuild:master Apr 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/break-core-cycle branch Apr 21, 2018

stuhood added a commit that referenced this pull request Apr 21, 2018

Break a Core / Node cycle (#5733)
### Problem

`Core` instances are currently being leaked in cases where `Nodes` have not completed running, and thus hold a `Context` in the closure for their `Future`. The cycle is:
```
Core -> Graph -> Node -> Context -> Core
```

### Solution

Clear all `Node` states when we `Drop` a `Scheduler`, breaking their cycles with the `Core`.

### Result

Fixes #5732 by ensuring that the `Store` held via `Scheduler -> Core -> Store` is dropped. It should also unblock #5611.
@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks for fixing this up!

Do you have any thoughts on how we can proactively avoid/detect Arc cycles in the future? This was a super easy problem to fall into, and I can believe we will again...

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 24, 2018

Do you have any thoughts on how we can proactively avoid/detect Arc cycles in the future? This was a super easy problem to fall into, and I can believe we will again...

I do not. My hope is that in the long run, this is the only usage of Arc in the whole codebase, because the rest end up replaced with references via async/await. Unclear whether that will be the case though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment