Re: On nsICancelableRunnable (or how bad decisions come back to haunt you)
On 13/03/2016 09:22, Kyle Huey wrote: > Much of the "disease" you see is simply support for DOM worker threads, > because every runnable that can run on those threads must be > cancelable. Workers do not guarantee one of the invariants that other > XPCOM threads do: that every runnable is eventually run after being > successfully dispatched. Instead we guarantee that every runnable is > either Run() or Cancel()ed. Yeah, that's something I had noticed and I was trying to understand why. So it seems that this interface actually turned out to have a proper use after all. > The disadvantage of pushing this into the event queue is that today > runnables that are dispatched from other threads to workers *must* > implement nsICancelableRunnable, which means authors of code that runs > on workers must think about this. We do already have an exception for > NS_DispatchToCurrentThread though (via ExternalRunnableWrapper), so this > may be something we can relax. It would generally require us to write > cleanup code (for the existing runnables and any future ones) to run > from destructors, and historically destructors could run on either the > dispatching thread or the executing thread (although that has changed > somewhat.) My gripes with its original design is that it's inherently non-thread-safe and non-atomic (i.e. Cancel() can be called while Run() is being executed) and it essentially drops the burden of implementing proper cancellation on the user. My idea of handling cancellation externally was motivated by having the operation be both thread-safe and fully atomic (either something runs or it doesn't). This however implies that if the task was fire-and-forget, then destruction will always be done by the canceling thread and will happen immediately; in the current scenario destruction will be done by the thread owning the queue, and it will happen only when the runnable is drained from it. > Regardless, your plan should probably involve me ;) Yeah, this was my idea when nsICancelableRunnable was practically only used in FxOS-specific code to support canceling potentially long-running operations posted on the main thread (and which might have not executed yet). Since it's use has evolved it might not be worth pursuing it anymore, which is why I wanted to hear about how it's been used now. > PS. Subscribe to the mailing list so you don't get caught in moderation. I'm already subscribed, I just verified it, it's bizarre that the message was held :-/ Gabriele signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: On nsICancelableRunnable (or how bad decisions come back to haunt you)
On Fri, Mar 11, 2016 at 7:04 AM, Gabriele Svelto
wrote:
> Many moons ago, while we were frantically trying to make Firefox OS 1.0
> run half-decently, I introduced a derived class of nsIRunnable that
> could be canceled: nsICancelableRunnable. As with many other things that
> led to the first release of FxOS this was done in a rush and without
> much thinking. It's only goal at the time was to allow me to cancel a
> single, simple, stateless runnable that could take a while to run.
>
> Thinking about it afterwards I came to the conclusion that this was a
> terrible approach: implementing the Cancel() method was up to the user
> and there was no telling on which threads Run() and Cancel() could be
> called making it potentially racy. So I thought it could be replaced by
> adding a Cancel() method to the event queue that would just remove the
> event in a thread-safe way. This seemed a cleaner, safer approach.
>
> But then I looked into our code and discovered that
> nsICancelableRunnable has spread like a disease through the codebase and
> it's systematically used in workers to let us cancel events posted to
> them. Ouch. Some of its uses highlight its problems: in some cases its
> only purpose is to force calling the Run() method by the calling thread
> so that certain objects get destroyed on that thread (possibly to
> prevent leaks). In other cases we have patterns like this:
>
> Run() {
> if (mDoSomething) {
> DoSomething();
> }
> }
>
> Cancel() {
> mDoSomething = false;
> }
>
> Which is fine if both methods are called by the same thread, but most
> likely not if they're called on different threads and the interface
> itself does nothing to prevent you from doing it.
>
> All in all I'd still like to get rid of it but I'm not sure if my
> original approach of moving the cancel functionality into the EventQueue
> is the right way to go. Which leads me to the point of this e-mail: to
> gather feedback on my idea to get rid of it. Since I don't have enough
> knowledge about all the affected areas I'd like to hear from those who
> actually used it.
On workers Run and Cancel are always called by the same thread, so there is
no race.
Much of the "disease" you see is simply support for DOM worker threads,
because every runnable that can run on those threads must be cancelable.
Workers do not guarantee one of the invariants that other XPCOM threads do:
that every runnable is eventually run after being successfully dispatched.
Instead we guarantee that every runnable is either Run() or Cancel()ed.
The disadvantage of pushing this into the event queue is that today
runnables that are dispatched from other threads to workers *must*
implement nsICancelableRunnable, which means authors of code that runs on
workers must think about this. We do already have an exception for
NS_DispatchToCurrentThread though (via ExternalRunnableWrapper), so this
may be something we can relax. It would generally require us to write
cleanup code (for the existing runnables and any future ones) to run from
destructors, and historically destructors could run on either the
dispatching thread or the executing thread (although that has changed
somewhat.)
Regardless, your plan should probably involve me ;)
- Kyle
PS. Subscribe to the mailing list so you don't get caught in moderation.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
On nsICancelableRunnable (or how bad decisions come back to haunt you)
Many moons ago, while we were frantically trying to make Firefox OS 1.0
run half-decently, I introduced a derived class of nsIRunnable that
could be canceled: nsICancelableRunnable. As with many other things that
led to the first release of FxOS this was done in a rush and without
much thinking. It's only goal at the time was to allow me to cancel a
single, simple, stateless runnable that could take a while to run.
Thinking about it afterwards I came to the conclusion that this was a
terrible approach: implementing the Cancel() method was up to the user
and there was no telling on which threads Run() and Cancel() could be
called making it potentially racy. So I thought it could be replaced by
adding a Cancel() method to the event queue that would just remove the
event in a thread-safe way. This seemed a cleaner, safer approach.
But then I looked into our code and discovered that
nsICancelableRunnable has spread like a disease through the codebase and
it's systematically used in workers to let us cancel events posted to
them. Ouch. Some of its uses highlight its problems: in some cases its
only purpose is to force calling the Run() method by the calling thread
so that certain objects get destroyed on that thread (possibly to
prevent leaks). In other cases we have patterns like this:
Run() {
if (mDoSomething) {
DoSomething();
}
}
Cancel() {
mDoSomething = false;
}
Which is fine if both methods are called by the same thread, but most
likely not if they're called on different threads and the interface
itself does nothing to prevent you from doing it.
All in all I'd still like to get rid of it but I'm not sure if my
original approach of moving the cancel functionality into the EventQueue
is the right way to go. Which leads me to the point of this e-mail: to
gather feedback on my idea to get rid of it. Since I don't have enough
knowledge about all the affected areas I'd like to hear from those who
actually used it.
Gabriele
signature.asc
Description: OpenPGP digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
