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

Fix DeliverUnit-related crash #17363

Merged
merged 1 commit into from Jan 1, 2020
Merged

Conversation

@reaperrr
Copy link
Contributor

reaperrr commented Nov 20, 2019

(Probably) fixes #17208.

@reaperrr reaperrr mentioned this pull request Nov 20, 2019
16 of 16 tasks complete
Copy link
Contributor

tovl left a comment

Code changes look good.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 21, 2019

Does this relate to #17353 at all?

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Nov 21, 2019

The crash report in #17208 indicates that DeliverUnit was the root activity and not related to FerryUnit.

@ghost

This comment has been minimized.

Copy link

ghost commented Nov 21, 2019

(Probably) fixes

See #17208 (comment) for reproduction information.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 21, 2019

@tovl I’m wondering whether there may be some more fundamental issue that then triggers the crashes here or in FerryUnit depending on which runs first.

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Nov 21, 2019

Hmm, there is certainly something weird going on. The only time DeliverUnit runs as root activity and not as a child of FerryUnit within D2K is when a new refinery is build and a new harvester gets delivered. It shouldn't be possible for anything to happen to the harvester between it being created and the DeliverUnit activity ticking that could make it null.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 21, 2019

My testing in #17208 shows that we need to be checking self.IsDead (and carryall.Carryable if we want to be extra safe) inside the FrameEndTask instead.

@reaperrr reaperrr force-pushed the reaperrr:fix-deliver-crash branch from 1a3d14b to 589d4d9 Nov 22, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Nov 22, 2019

Updated.

@pchote pchote added this to the Next Release milestone Dec 15, 2019
@pchote
pchote approved these changes Dec 28, 2019
Copy link
Member

pchote left a comment

LGTM

@pchote pchote added the PR: Needs +2 label Dec 28, 2019
Copy link
Member

abcdefg30 left a comment

Code changes look reasonable.

@abcdefg30 abcdefg30 merged commit 804d61a into OpenRA:bleed Jan 1, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Jan 1, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.