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

Add plumbing for performing custom activity code on actor disposal #15622

Merged
merged 1 commit into from Sep 24, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Sep 17, 2018

It appears that if an actor dies (or is disposed directly) while an activity is active, the actor will be disposed before the activities' TickOuter can tick one last time, so OnLastRun is never triggered.
As #15543 shows, this can lead to some nasty bugs.

This PR enforces a proper activity end on actor death/disposal.

/// <summary>
/// Used only by Actor.Dispose() to make sure the activity finishes properly on death/disposal of actor
/// </summary>
public virtual void ForceCancel(Actor self)

This comment has been minimized.

@pchote

pchote Sep 17, 2018

Member

I think it would be better to call this OnActorDispose instead, and leave the default implementation doing nothing.

The individual activity subclasses can then choose to override this with their own cleanup logic, avoiding any side-effects that might otherwise happen by blindly calling OnLastRun without knowing what the activities might be doing inside there.

This comment has been minimized.

@reaperrr

reaperrr Sep 17, 2018

Contributor

I can do that, but I'm not convinced that OnLastRun being skipped on actor death/disposal is intentional.
Pinging @obrakmann just in case he has time to chime in on this.

This comment has been minimized.

@obrakmann

obrakmann Sep 17, 2018

Contributor

Tbh, I didn't quite think of modifying third-party state in the OnLastRun method, so the thought hasn't even crossed my mind. But I do agree with Paul here that unconditionally running OnLastRun might not be such a hot idea, and it should be up the activities themselves to decide what needs cleaning up after an actor gets disposed of.

@reaperrr reaperrr force-pushed the reaperrr:fix-act-onlastrun branch from d747f13 to a701bdc Sep 17, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 17, 2018

Ok, somehow can't get the fix to work anymore no matter what I try, I need to debug that first.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 17, 2018

My first thought is the need to be careful about parent/child activities.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 17, 2018

Yes, the problem was that ResupplyAircraft chains Repair as ChildActivity.

This PR should be good as-is, I didn't need to make any further changes to this PR's code to fix/work around the problem in #15543 (which I've also updated now, so it serves as testcase).

public virtual bool Cancel(Actor self, bool keepQueue = false)
{
// Just a safe-guard in case this activity was already successfully cancelled
if (IsCanceled)

This comment has been minimized.

@pchote

pchote Sep 17, 2018

Member

This will cause problems if you cancel an activity that takes a few ticks to complete (e.g. Move), queue a new activity, and then call Cancel again before the original activity has completed. Expected behaviour: second activity is cancelled, actual behaviour: it won't be.

@reaperrr reaperrr force-pushed the reaperrr:fix-act-onlastrun branch from a701bdc to 39323e3 Sep 20, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 20, 2018

Updated.
That Cancel change was only a left-over from my old ForceCancel approach anyway.

// If CurrentActivity isn't null, run OnActorDispose in case some cleanups are needed.
// This should be done before the FrameEndTask to avoid dependency issues.
if (CurrentActivity != null)
CurrentActivity.RootActivity.OnActorDispose(this);

This comment has been minimized.

@pchote

pchote Sep 20, 2018

Member

Do we need to worry about propagating this to child activities, or do we assume that the activities will handle this themselves if they use them? My gut feeling is that this probably should be automatic.

This comment has been minimized.

@reaperrr

reaperrr Sep 20, 2018

Contributor

I lean towards agreeing that it should be automatic, that would require changing the protection level of ChildActivity, though.
Personally I wouldn't really mind, just wanted to point it out in case there are some potential issues with that I'm not aware of.

This comment has been minimized.

@pchote

pchote Sep 20, 2018

Member

One option is to define a OnActorDisposeOuter on Activity (it should have internal visibility so that mod code can't see it), which calls OnActorDisposeOuter on the child activity if it exists and then calls OnActorDispose on itself. We use this pattern in other places, notably widgets. We may also want to propagate this through the NextActivity chain too?

If this isn't going to work then I think a reasonable compromise is to add a comment to the default OnActorDispose definition explaining that it must be manually chained through by any activities that use child activities.

This comment has been minimized.

@reaperrr

reaperrr Sep 21, 2018

Contributor

Appears to work fine, so I'd like to go with it.

Not sure what you mean with propagating through the NextActivity chain, though?

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 21, 2018

Member

I suppose calling OnActorDisposeOuter on NextActivity (and that activity will call it on its NextActivity and so on, hence "through the chain").

This comment has been minimized.

@reaperrr

reaperrr Sep 21, 2018

Contributor

But do we really want or need that?

When the actor is disposed, NextActivity hasn't even started yet, or it wouldn't be "Next" anymore. I don't see why we would want to run the dispose-specific code on activities that haven't even started yet, since that would mean OnFirstRun hasn't run, either. I'm not even sure if the ctor is run before the activity becomes 'current'.

This comment has been minimized.

@reaperrr

reaperrr Sep 22, 2018

Contributor

Leaving changes to NextActivity for the future and opened #15632 instead, as discussed on IRC.

@reaperrr reaperrr changed the title Fix Activity.OnLastRun being skipped when actor dies Add plumbing for performing custom activity code on actor disposal Sep 20, 2018

Add OnActorDispose plumbing to Activity
This allows activities to perform necessary cleanups on actor
death/disposal, for example by running OnLastRun directly,
which would otherwise be skipped when the actor dies or is disposed
through other means.
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 22, 2018

Updated.

@abcdefg30 abcdefg30 merged commit 9bcb754 into OpenRA:bleed Sep 24, 2018

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 Sep 24, 2018

@reaperrr reaperrr deleted the reaperrr:fix-act-onlastrun branch Oct 26, 2018

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