291252 - calIcalendar getItems, calIOperationListener onGetItems: Need to specify thread behavior of multiple calls?

Status: NEW

(Calendar :: Internal Components, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: trunk

The calICalendar getItems method says it may call the listener
onGetResult method multiple times (followed by a call to onOperationComplete):
246    * The results of the operation are reported through the listener,
247    * via zero or more onGetResult calls followed by an onOperationComplete.
http://lxr.mozilla.org/seamonkey/source/calendar/base/public/calICalendar.idl#246

The calIOperationListener onGetItems method says it may be called
multiple times:
376    * Multiple onGetResults might be called 
http://lxr.mozilla.org/seamonkey/source/calendar/base/public/calICalendar.idl#376

However, neither specify the threading behavior of the calls.
Multiple calls might be used to enable the listener to begin
processing early results before later results arrive, so processing
could be overlapped with fetching.  If fetching is multithreaded (say,
a thread for each remote calendar), then the multiple calls could be
multithreaded and asynchronous (they could overlap in time, later
calls to onGetItems arriving before earlier calls finish, so listener always
gets information as soon as possible).

Some alternative possible approaches:

A. Provider synchronizes: provider must call listener synchronously
   (each one waiting for the previous call to end before starting).
   It is up to the provider to use locking if necessary.  

   (Provider could wrap listener with a locking guard.)

B. Manager synchronizes: The listener is automatically guarded with a
   Lock before it is passed to any provider implementation, so even if
   multiple concurrent calls are made by different threads, only one
   may run at a time, and later calls will wait until earlier ones
   finish.

   (When calendar manager returns a provider, the provider is wrapped
   with a manager proxy so that the proxy can wrap the listener with
   a guard in any calls to the provider that pass a listener.)

C. Listener synchronizes: Multiple threads may call the listener
   asynchronously, so listeners will be informed as soon as additional
   data is available.  Listeners must use some concurrency control,
   such as a lock, to protect their internal state from erroneous
   modifications due to asynchronous calls.

   (I don't think JavaScript 1.5 has locks, so JavaScript listeners
   probably have to use NPSR locks --- is that possible?  Are
   JavaScript threads implemented with OS threads?  If multiple
   JavaScript threads are implemented with just one OS thread, NPSR
   locks might not work for JavaScript threads.)

D. Provider synchronizes unless Listener synchronizes: If
   listener.isThreadSafe is not true, the provider wraps the listener
   with a guard as in 'A'.  If listener.isThreadSafe is true, then it
   is not wrapped, so it can behave as 'C'.

   (Add an attribute "isThreadSafe" to the calIOperationListener
   interface.  Implementations may provide a default value of false.)

E. Manager synchronizes unless Listener synchronizes: If
   listener.isThreadSafe is not true, the manager wraps the listener
   with a guard as in 'B'.  If listener.isThreadSafe is true,
   then it is not wrapped, so it can behave as 'C'.

   (Add an attribute "isThreadSafe" to the calIOperationListener
   interface.  Implementations may provide a default value of false.
   When calendar manager returns a provider, the provider is wrapped
   with a manager proxy so that the proxy can wrap the listener with
   a guard in any calls to the provider that pass a listener where
   listener.isThreadSafe is not true.)


Reproducible: Always

Steps to Reproduce:

Comment 1

[Mike Shaver (:shaver emeritus)]

I think that providers should probably guarantee that the listeners are called
back on the thread that made the call into the provider.  Beyond that, I'm not
sure that we need to specify any other behaviour, explicitly.  The asynchrony
you describe -- which we value highly, even at some non-trivial complexity cost
-- doesn't require multiple threads to be in play.  We get it today with the
CalDAV provider, I believe, by simply relying on the HTTP engine to call us back
asynchronously with results, etc.

(Because of the nature of XML parsing, CalDAV actually gets all the results for
a given operation in a burst of calls at the end, but if someone should swoop
down from heaven and give us a way to sanely do incremental XML parsing, nothing
should need to change on the CalDAV side to accomodate it.)

Comment 2

[gekacheka]

Requiring the provider to call using the same thread seems more
restrictive than requiring the provider to synchronize, but it's one
way to specify calls to the listener are synchronized.  Is it
specifically for the JavaScript engine?

(One case where concurrency issues might come up: doing event layout
 in the grid views.  [Maybe similar concurrency issues to doing html
 layout?]  Cohorts of events overlapping in time must be re-laid out
 when new events are added to the cohort.  

 * The one-time approach is to simply wait until all events arrive.
   However, that does not provide incremental display from a slow link.
 * The redo approach is to re-layout everything, or at
   least the changed cohorts, after each onGetItems is received.
   However, since later onGetItems calls may supply additional events
   to cohorts being laid out, it would be faster to incorporate
   the new information as soon as it arrives rather than finish
   an obsolete layout.  Might be particularly bad if some provider
   provides one event at a time.
 * The interruptable approach is to abort the layout of events (or of a
   cohort) soon after a new onGetItems arrives with additional events
   (or additional events for the cohort) and restart the layout of
   events (or the changed cohort(s)) with the additional information.

 So if onGetItems is called only from one thread, then if the
 onGetItems thread is to interrupt layout with new data, the layout
 must be done in another thread.  [It looks like this is difficult to
 do in JavaScript because of the lack of locks or other
 synchronization mechanisms, but that should not limit the interface.
 (I didn't find any examples of locks in mozilla js code via lxr.)])

(What causes the CalDav parser to consume all xml before producing
 anything?  Is it because it is validating, or because it is building a
 DOM?  A stream parser like Expat, perhaps with a SAX interface, should
 be able to provide incremental parsing.)

Comment 3

[Mike Shaver (:shaver emeritus)]

No, the JS engine can and does run happily on multiple threads.  And if the
listener for some reason wants to process the events in parallel (I'm having
trouble thinking of a plausible case for that, I confess), they are free to
proxy calls over to other threads.

Doing event layout in the views is a case in which the caller definitely wants
to be called back on the same thread, as poking into the layout engine on a
thread other than the UI thread is a recipe for disaster.  And calling back on
the same thread gives the same scheduling behaviour that the caller would have
had in a simpler world in which we didn't have to worry about starving the UI,
which I think makes for a much clearer programming model.

You don't want to hold locks for very long in the Mozilla world, but instead to
use event queues, which are usable from JavaScript.  "Interrupting" another
thread requires that thread to check for some updated state, which can be a
property on a JS object (implicitly threadsafe, though some higher-level
operations are not atomic) or via an event queue posting.  I don't think that
interrupting event layout is interesting here, certainly not enough to
complicate the interface and impose complex thread-management requirements on
all callers and providers -- and it's still a very thorny issue for Gecko's much
more intricate HTML engine, whose semantics are going to gate any "optimally
efficient" interruptible layout of events.

(The WebDAV result parser operates on a DOM, because building correct logic for
WebDAV responses on the basis of a SAX-like interface is quite a bit more work.
 If you want to make that work, the CalDAV provider could then easily become
incremental as well, but I believe that WebDAV requires us to not process any
part of an invalid response document.  This is a similar problem to doing
incremental layout of XHTML, so you can probably find more information on bugs
on that topic.)

This bug was well-summarized (and well-considered; thank you for raising this
issue!): we need to specify the threading behaviour of calIOperationListener's
callback invocations, and I believe the correct specification is "the provider
must invoke the listener's callback on the same thread used to initiate the
operation on which it is reporting".  Providers who wish to employ multiple
threads in their implementation can do so as long as they proxy back to the
original thread when they're returning results -- I believe this is also how
necko specifies the callback behaviour of protocol implementations. 
Hypothetical (and, IMO, quite unlikely) callers are free to proxy their call to
another thread in order to receive notifications on that thread, or to proxy
individual results to other threads for parallel processing.

I'd love to see a patch incorporating the quote above, or a longer version of
it, into the calICalendar and calIOperationListener documentation.  (And bugs
filed on providers that don't honour the new specification, though I think we're
OK with all our current providers.)

We should also solve this for calIObserver and calICalendar::addObserver, but
let's do that in another bug referencing this one.

Comment 4

[gekacheka]

Thanks for the detailed explanation.

Would like doc to give example JS code, but nsIProxyObjectManager does not
define constants it needs (filed bug 292036).

(Events are laid out explicitly in a stacked layer after the grid is laid out by
html layout, so while html layout does lay out the grid, it doesn't play an
active role during event layout.)

(It looks like incremental parsing of ics wouldn't be too hard if all timezones
were defined before they were used... it could emit objects as it parses each
VEVENT as long as all timezones are known.  If the VEVENT references a timezone
which is defined later in the file/message, then the object would have to be
held until the timezone is defined.)

Comment 5

[Mike Shaver (:shaver emeritus)]

I don't think we need sample code in the IDL (I'd rather see a doc on devmo
describing how to work with the calendar interfaces, and you should read "rather
see" as "lust after, perhaps), but that lack of constants is indeed sucky. 
Leave the dep, as there's more to documenting these interfaces well than just
the usual terse IDL comments, and your catch of the missing constants is a good one.

HTML parsing doesn't affect us, but the general problem of interrupting layout
of events (which can be in the middle of Gecko reflow when an interrupt is
desired) applies, I think, to your hypothetical case as well as it does to the
Gecko large-page problem.

Incrementally parsing ICS is indeed not a hard design problem, we just don't
have a library that does it, or anyone signed up to make libical into that library.

Comment 6

[Mike Shaver (:shaver emeritus)]

Unfortunately, I do not have cycles to work on Calendar stuff these days (just as it's getting to the good part!), so I am a bad owner for these bugs.  To delete the tragically-large chunk of bugspam, search for gregorianabdication.