Ordering shutdown observers?

[Vladan Djeric]

Hi all,

I recently came across a situation where it would have been useful to
control the order in which components receive the same shutdown
notification. Specifically, Telemetry writes out session data to the
profile dir during the "profile-before-change" event, but it needs to
include any measurements reported by other components while they are
handling the same "profile-before-change" notification. So ideally,
Telemetry would be the last observer to get the notification.

As an additional example, OS.File needs to be the second-last observer
of "profile-before-change", since it needs to service new requests to
OS.File from other components during their handling of
"profile-before-change", but it also needs to record measurements in
Telemetry before Telemetry has handled the same event.

I'd like to know if these use-cases are sufficiently rare that we should
just add new shutdown events when needed (e.g. we added
"profile-before-change2" for Telemetry in bug 844331), or if we should
come up with a general way to impose order of shutdown based on
dependencies?

Thanks,
Vladan

[Gregory Szorc]

There are race conditions all over the place. Another example is
preferences. Preferences flush during one of the shutdown observers
IIRC. If your observer mutates preferences after preferences has fired,
pref writes are lost. So, it all comes down to relying on the order
observers were registered on startup (since observers are notified in
registration order). This all seems rather fragile to me.

I think the more compelling use case is service startup. Proper
dependencies should allow us to more intelligently start services on
demand. This should lead to lower resource utilization and faster
startup times. Shutdown times should also speed up if there are fewer
services to shut down.

[Ehsan Akhgari]

On 2013-05-15 5:18 PM, Vladan Djeric wrote:
> I'd like to know if these use-cases are sufficiently rare that we should
> just add new shutdown events when needed (e.g. we added
> "profile-before-change2" for Telemetry in bug 844331), or if we should
> come up with a general way to impose order of shutdown based on
> dependencies?

Do you have use cases besides these two?

Ehsan

[Trevor Saunders]

I believe roc proposed just having an explicit hard coded list of things
to start up a while ago, and I'm tempted to say that's what we should do for
shutdown too. So just add an explicit call to some os.file thing
followd by a call to a telemetry function after profile-before-change
but not after anything else.

Trev

>
> Thanks,
> Vladan
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Justin Lebar]

> I believe roc proposed just having an explicit hard coded list of things
> to start up a while ago, and I'm tempted to say that's what we should do for
> shutdown too. So just add an explicit call to some os.file thing
> followd by a call to a telemetry function after profile-before-change
> but not after anything else.

Indeed, to implement ClearOnShutdown, I added an explicit call to
KillClearOnShutdown during the XPCOM shutdown process, precisely
because I needed to guarantee a particular ordering wrt shutdown
observers.

[L. David Baron]

On Wednesday 2013-05-15 14:32 -0700, Gregory Szorc wrote:
> I think the more compelling use case is service startup. Proper
> dependencies should allow us to more intelligently start services on
> demand. This should lead to lower resource utilization and faster
> startup times. Shutdown times should also speed up if there are
> fewer services to shut down.

This is what we do already; we don't create an XPCOM service until
somebody asks for it. Now, I'm not saying that all of our code is
perfect about not *asking* for the service until it's needed. But
in many cases that's more trouble than it's worth; there are many
things we know we'll need during startup, and it's not worth the
extra overhead of checking every time if we've already called
getService.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla http://www.mozilla.org/ 𝄂

[Gregory Szorc]

On 5/15/13 3:31 PM, L. David Baron wrote:
> On Wednesday 2013-05-15 14:32 -0700, Gregory Szorc wrote:
>> I think the more compelling use case is service startup. Proper
>> dependencies should allow us to more intelligently start services on
>> demand. This should lead to lower resource utilization and faster
>> startup times. Shutdown times should also speed up if there are
>> fewer services to shut down.
>
> This is what we do already; we don't create an XPCOM service until
> somebody asks for it. Now, I'm not saying that all of our code is
> perfect about not *asking* for the service until it's needed. But
> in many cases that's more trouble than it's worth; there are many
> things we know we'll need during startup, and it's not worth the
> extra overhead of checking every time if we've already called
> getService.

Ahh, I was thinking more of JS services. In this world, your manifest
adds an entry to the "app-startup" category and your service receives
the "app-startup" notification. It is customary for it to register an
observer for a later startup phase and finish initialization then.

Many services also install one shot timers so they don't overburden app
init.

Typically if there are service dependencies, you have service A emit a
notification and service B then starts upon receiving it. A problem with
this model is you can't reliably and/or easily init service B by itself
because B doesn't necessary know how to init A.

The current model is essentially a giant domino reaction cascaded from
app-startup. I think I'd prefer a "pull based" solution where the first
consumer of service B (perhaps it's a timer or an observer if B is
frequently-used) tries to grab an instance and this results in service A
being instantiated on-demand. This is accomplished via a service
management framework of some kind that knows about all services and
their dependencies.

This is comparable to how *NIX operating systems initialize. Gecko is
currently using System-V style runlevels. But all the new hotness are
replacements like sytemd and upstart which explicitly express service
dependencies, can start services in parallel and on demand, and are much
faster at starting the operating system as a result.

[Robert O'Callahan]

On Thu, May 16, 2013 at 11:00 AM, Gregory Szorc <g...@mozilla.com> wrote:

> This is comparable to how *NIX operating systems initialize. Gecko is
> currently using System-V style runlevels. But all the new hotness are
> replacements like sytemd and upstart which explicitly express service
> dependencies, can start services in parallel and on demand, and are much
> faster at starting the operating system as a result.

That hotness makes sense in an environment where there's a very large
number of possible configurations. But Gecko isn't like that.

Rob
--
q“qIqfq qyqoquq qlqoqvqeq qtqhqoqsqeq qwqhqoq qlqoqvqeq qyqoquq,q qwqhqaqtq
qcqrqeqdqiqtq qiqsq qtqhqaqtq qtqoq qyqoquq?q qEqvqeqnq qsqiqnqnqeqrqsq
qlqoqvqeq qtqhqoqsqeq qwqhqoq qlqoqvqeq qtqhqeqmq.q qAqnqdq qiqfq qyqoquq
qdqoq qgqoqoqdq qtqoq qtqhqoqsqeq qwqhqoq qaqrqeq qgqoqoqdq qtqoq qyqoquq,q
qwqhqaqtq qcqrqeqdqiqtq qiqsq qtqhqaqtq qtqoq qyqoquq?q qEqvqeqnq
qsqiqnqnqeqrqsq qdqoq qtqhqaqtq.q"

[Neil]

Gregory Szorc wrote:

Ahh, I was thinking more of JS services. In this world, your manifest adds an entry to the "app-startup" category and your service receives the "app-startup" notification. It is customary for it to register an observer for a later startup phase and finish initialization then.

Well, if it's still at startup then all you're doing is moving the service load to a different point in startup, but you can register with profile-after-change instead if that's early enough. (I don't think any other notifications have a category.)

-- 
Warning: May contain traces of nuts.

[Gavin Sharp]

On Wed, May 15, 2013 at 4:00 PM, Gregory Szorc <g...@mozilla.com> wrote:
> Ahh, I was thinking more of JS services. In this world, your manifest adds
> an entry to the "app-startup" category and your service receives the
> "app-startup" notification. It is customary for it to register an observer
> for a later startup phase and finish initialization then.

We're on a tangent here, but many (most?) such services only care to
be instantiated so that they can in turn observe other events (or they
need to be continuously observing events from other services). It's
sometimes wasteful to load+initialize the entire service just to
observe events, so a mechanism similar to the existing
messageWakeupService.js (but for observer service notifications rather
than message manager messages) could be useful in avoiding some of
that waste.

Gavin

[Gabriele Svelto]

> Do you have use cases besides these two?

In bug 845190 [1] we're seeing an issue which might be caused by an
unexpected shutdown sequence. What seems to be happening is that a DOM
worker thread is created very late during the test; once ShutdownXPCOM()
is invoked the worker runtime service is supposed to tear down all
remaining workers but in order to do so it has to wait for pending ones
to finish loading and then stop. Unfortunately the said worker starts
loading when many other subsystems have already been shut down (either
by receiving a shutdown-xpcom or shutdown-xpcom-threads event) and trips
over errors/assertions because of that.

Gabriele

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=845190

[Ehsan Akhgari]

Is this not the OS.File issue that Vladan mentioned?

My point is that there doesn't seem to be enough use cases to warrant a
new infrastructure to handle shutdown dependencies.

Ehsan

[Philipp Kewisch]

I agree, proper dependencies here would be a win. For Calendar/Lightning
we have implemented our own startup service that starts up the calendar
components in order. Having a startup/shutdown service would be very
nice since we can wait for startup of important mail components, then
load/unload our calendar components in order.

Philipp

[Gabriele Svelto]

On 16/05/2013 15:03, Ehsan Akhgari wrote:
> Is this not the OS.File issue that Vladan mentioned?

Yes but it seems to me that it's not an OS.File-specific issue. Anything
using workers might run into shutdown problems if the same situation
arises (pending but not-yet-loaded worker requiring an already
terminated service during shutdown).

> My point is that there doesn't seem to be enough use cases to warrant a
> new infrastructure to handle shutdown dependencies.

I would say it's a matter of how complicated would it be to bolt it on
our current infrastructure. Having a more robust shutdown procedure
certainly doesn't hurt if it doesn't require too much effort to add it.

Gabriele

[David Rajchenbach-Teller]

On 5/16/13 3:03 PM, Ehsan Akhgari wrote:
> Is this not the OS.File issue that Vladan mentioned?
>

> My point is that there doesn't seem to be enough use cases to warrant a
> new infrastructure to handle shutdown dependencies.

Well, as we expand our use of OS.File, we start observing a number of
issues, most of which do not seem to be due to OS.File itself, but more
generally to (chrome) workers.

Here are a few:
- clients of OS.File need to write their data before OS.File shuts down
– that's Vladan's remark;
- JS Workers (including OS.File's I/O worker) need to be properly
initialized before shut down or to cancel themselves nicely upon
shutdown – that's Gabriele's remark;
- OS.File itself needs to be informed of shut down to (asynchronously)
collect information and print warnings about leaking file descriptors,
and also to start rejecting additional requests.

That's from the top of my head, I am sure that I am missing a few.

As we move as much code as possible to workers/threads, I believe that
we are going to suffer from a growing number of such issues. So, yes, I
am convinced that we need a way to handle dependencies.

Moreover, I believe that we need to make dependencies somewhat explicit,
otherwise we will at some point end up with unsatisfiable implicit
dependencies and we will need large refactorings to get around these.

Cheers,
David

--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[Ehsan Akhgari]

On 2013-05-16 10:23 AM, David Rajchenbach-Teller wrote:
> On 5/16/13 3:03 PM, Ehsan Akhgari wrote:
>> Is this not the OS.File issue that Vladan mentioned?
>>
>> My point is that there doesn't seem to be enough use cases to warrant a
>> new infrastructure to handle shutdown dependencies.
>
> Well, as we expand our use of OS.File, we start observing a number of
> issues, most of which do not seem to be due to OS.File itself, but more
> generally to (chrome) workers.
>
> Here are a few:
> - clients of OS.File need to write their data before OS.File shuts down
> – that's Vladan's remark;
> - JS Workers (including OS.File's I/O worker) need to be properly
> initialized before shut down or to cancel themselves nicely upon
> shutdown – that's Gabriele's remark;
> - OS.File itself needs to be informed of shut down to (asynchronously)
> collect information and print warnings about leaking file descriptors,
> and also to start rejecting additional requests.

If OS.File can detect shutdown at the correct time, then it seems like
all of the above issues are going to be easy to fix. Which makes me
think of all of these as one use case (please correct me if I'm wrong.)

> That's from the top of my head, I am sure that I am missing a few.
>
> As we move as much code as possible to workers/threads, I believe that
> we are going to suffer from a growing number of such issues. So, yes, I
> am convinced that we need a way to handle dependencies.
>
> Moreover, I believe that we need to make dependencies somewhat explicit,
> otherwise we will at some point end up with unsatisfiable implicit
> dependencies and we will need large refactorings to get around these.

A dependency system for startup/shutdown isn't free. It's going to have
runtime cost, and it's going to make the code more complicated. Given
the fact that we've managed to get by just fine without one for years, I
think we should continue to do that, and in the cases where you want
specific ordering, just use hard-coded notifications (such as
profile-before-change2 -- BTW, that's a terrible name!).

Cheers,
Ehsan

[Honza Bambas]

On 5/16/2013 8:12 PM, Ehsan Akhgari wrote:
> A dependency system for startup/shutdown isn't free. It's going to
> have runtime cost, and it's going to make the code more complicated.
> Given the fact that we've managed to get by just fine without one for
> years, I think we should continue to do that, and in the cases where
> you want specific ordering, just use hard-coded notifications (such as
> profile-before-change2 -- BTW, that's a terrible name!).

As I understand, the original problem is that we instantiate a service
for the first time "too late". It then is not able to shut it self down
using "xpcom-shutodown" notification, since it has been created from
"xpcom-shutodown" call.

IMO, enough would be to have XPCOM bool isAfterShutdown() flag
somewhere. Services that depends on xpcom-shutdown notification would
check for this flag and refuse to instantiate or init.

Could also be integrated in the generic constructor somehow -
constructors would express that instantiation may not happen after
certain events of xpcom.

-hb-

[Gabriele Svelto]

On 16/05/2013 20:12, Ehsan Akhgari wrote:
> If OS.File can detect shutdown at the correct time, then it seems like
> all of the above issues are going to be easy to fix. Which makes me
> think of all of these as one use case (please correct me if I'm wrong.)

David prepared a patch for bug 845190 which would fix shutdown problems
for OS.File worker threads that would already have been properly loaded
by the time ShutdownXPCOM() is invoked. The trouble is that some workers
might be loaded after that and so it's impossible to work around the
issue within OS.File code. The only quick fix would be using yet another
event for shutdown as you suggested.

If we go down that road we should at least find a way to document what
every event is for. I remember seeing a wiki page some time ago on the
topic but I'm not even sure if it contained up-to-date information or not.

Gabriele

[Brian Smith]

Ehsan Akhgari wrote:

> On 2013-05-15 5:18 PM, Vladan Djeric wrote:
> > I'd like to know if these use-cases are sufficiently rare that we
> > should just add new shutdown events when needed (e.g. we added
> > "profile-before-change2" for Telemetry in bug 844331), or if we
> > should come up with a general way to impose order of shutdown
> > based on dependencies?
>

> Do you have use cases besides these two?

Many things (and an increasing number) depend on PSM/NSS and the PSM team (a long time ago) implemented its own shutdown event registration scheme (nsNSSShutDownObject in nsNSSShutDown.h). There seems like there is at least one race due to NSS being shut down while things are still using NSS which is causing a crash or worse (presumably because there is not enough awareness of the need to implement nsNSSShutDownObject and/or it is too error-prone to do so). Also, NSS must be shut down in profile-before-change because it may write to the profile directory.

So, basically the nsNSSShutDownObject scheme is a variant of the "explicit dependencies" scheme and, not a very successful one. Perhaps there are other variants of explicit dependency schemes that would be less error prone, but I am skeptical. In general, generic dependency schemes of the "upstart" variety seem like very complicated solutions, considering we have global knowledge of all the components of Firefox that we could just hard-code in, if we can assume that addons do not affect the ordering.

Cheers,
Brian