[pytorch] Make RRefContext singleton leaky, deal with module destruct order race. #30172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… order race. RRefContext is a conventional singleton, used by rref.cpp. At module teardown time, it's not defined whether rref_context.cpp or rref.cpp will be destroyed first. We were observing a SIGSEGV because RRefContext is destroyed before a dangling ~UserRRef() call is able to execute. Particularly, the underlying ctx.agent()->getWorkerInfo(ownerId_) call failed. This change just avoids the SIGSEGV by forcing an intentional leak, though we still need to deal with why there's a dangling UserRref at module destruction time. Differential Revision: [D18620786](https://our.internmc.facebook.com/intern/diff/D18620786/) [ghstack-poisoned]
jjlilley
requested review from
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
jjlilley
pushed a commit
that referenced
this pull request
Nov 20, 2019
… order race. RRefContext is a conventional singleton, used by rref.cpp. At module teardown time, it's not defined whether rref_context.cpp or rref.cpp will be destroyed first. We were observing a SIGSEGV because RRefContext is destroyed before a dangling ~UserRRef() call is able to execute. Particularly, the underlying ctx.agent()->getWorkerInfo(ownerId_) call failed. This change just avoids the SIGSEGV by forcing an intentional leak, though we still need to deal with why there's a dangling UserRref at module destruction time. Differential Revision: [D18620786](https://our.internmc.facebook.com/intern/diff/D18620786/) ghstack-source-id: 94287441 Pull Request resolved: #30172
zhaojuanmao
approved these changes
Nov 20, 2019
Contributor
|
please still wait for tests to finish before landing |
Author
|
Thanks. The other, broader singleton change is having a harder time passing tests, so I wanted to try to just land the minimal version. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
RRefContext is a conventional singleton, used by rref.cpp. At module teardown
time, it's not defined whether rref_context.cpp or rref.cpp will be destroyed first.
We were observing a SIGSEGV because RRefContext is destroyed before a dangling
~UserRRef() call is able to execute. Particularly, the underlying
ctx.agent()->getWorkerInfo(ownerId_) call failed.
This change just avoids the SIGSEGV by forcing an intentional leak, though we still
need to deal with why there's a dangling UserRref at module destruction time.
Differential Revision: D18620786