1223766 - Removing an actor from an actor pool should destroy it.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Eddy Bruel [:ejpbruel]]

The way we deal with actor lifetimes is currently inconsistent, in the sense that removing an actor pool will destroy all actors in that pool, but removing a single actor from the pool will not destroy it.

This is somewhat unfortunate when the lifetime of a particular actor doesn't match that of any other actor: in that case, we either have to create a separate pool, containing only that single actor, or destroy the actor explicitly.

Note that this does not lead to problems when moving an actor from one pool to another: adding the actor to the new pool will remove it from the old one, without destroying it.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Removing an actor from an actor pool should destroy it.

Note that this patch causes test failures in test_promises_object_timetosettle-02.js because that test is broken. See bug 1207702 for details.

Comment 2

[Eddy Bruel [:ejpbruel]]

Try run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d0abac7b85

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8686000 [details] [diff] [review]
Removing an actor from an actor pool should destroy it.

Review of attachment 8686000 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Eddy! From what I can tell your patch looks OK, but please eventually run it by treeherder to make sure it won't break any tests.

I don't feel very comfortable reviewing this actor-lifetime change in the limited time I have, so I'll forward the rest of the review to Alex. Please direct any follow-up review requests to him as well.

Also, I think you forgot a call site[0] of `ActorPool.cleanup()`.

[0] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/promises.js#229

::: devtools/server/actors/common.js
@@ +217,5 @@
>  }
>  
>  ActorPool.prototype = {
>    /**
> +   * Run all actor cleanups.

Nit: "Remove all remaining actors"?

::: devtools/server/main.js
@@ +1386,5 @@
>      let index = this._extraPools.lastIndexOf(aActorPool);
>      if (index > -1) {
>        let pool = this._extraPools.splice(index, 1);
>        if (!aNoCleanup) {
> +        pool.map(function(p) { p.destroy(); });

Nit: I find it strange to call `map()` on an array that will only ever have one element.

What about `let pool = this._e.splice(index, 1)[0]` and then `pool.destroy()`?

Comment 5

[Jan Keromnes [:janx]]

(P.S. Nevermind my comment about try runs, I see you already sent one.)

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Removing an actor from an actor pool should destroy it.

New patch with nits by Jan addressed.

Comment 7

[Jan Keromnes [:janx]]

Comment on attachment 8686464 [details] [diff] [review]
Removing an actor from an actor pool should destroy it.

Alex took the day off. Panos, could you please have a look instead? I already pre-reviewed the patch, but I'd love feedback from someone who knows actor/pool lifetimes better than me.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8686464 [details] [diff] [review]
Removing an actor from an actor pool should destroy it.

Review of attachment 8686464 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8686464 [details] [diff] [review]
Removing an actor from an actor pool should destroy it.

Review of attachment 8686464 [details] [diff] [review]:
-----------------------------------------------------------------

I believe you can also remove the use of `cleanup` from protocol.js:

https://dxr.mozilla.org/mozilla-central/source/devtools/server/protocol.js#833

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3da6ad8bb8a9

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3da6ad8bb8a9