Removing an actor from an actor pool should destroy it.

RESOLVED FIXED in Firefox 45

Status

()

Firefox
Developer Tools
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8686000 [details] [diff] [review]
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.
Attachment #8686000 - Flags: review?(janx)
(Assignee)

Updated

2 years ago
Depends on: 1207702
(Assignee)

Comment 2

2 years ago
Try run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d0abac7b85
(Assignee)

Updated

2 years ago
Duplicate of this bug: 781397
(Assignee)

Updated

2 years ago
Blocks: 1221892
(Assignee)

Updated

2 years ago
Blocks: 1212797, 1214248

Comment 4

2 years ago
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()`?
Attachment #8686000 - Flags: review?(poirot.alex)
Attachment #8686000 - Flags: review?(janx)
Attachment #8686000 - Flags: feedback+

Comment 5

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

Comment 6

2 years ago
Created attachment 8686464 [details] [diff] [review]
Removing an actor from an actor pool should destroy it.

New patch with nits by Jan addressed.
Attachment #8686000 - Attachment is obsolete: true
Attachment #8686000 - Flags: review?(poirot.alex)
Attachment #8686464 - Flags: review?(poirot.alex)

Comment 7

2 years ago
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.
Attachment #8686464 - Flags: review?(poirot.alex) → review?(past)
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!
Attachment #8686464 - Flags: review?(past) → review+
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

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3da6ad8bb8a9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.