720653 - WebSMS Android backend: nsISmsDatabaseService::saveSentMessage should not be called synchronous

Status: RESOLVED INVALID

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

There are some leaky abstractions in the interplay between nsISmsService::send() and nsISmsDatabaseService::saveSentMessage(). In particular, the interfaces assume that an nsISmsService::send() implementation will look something like this:

  function send(number, message, requestId, processId) {
    doSendTextMessage(number, message, function (result) {
      // ... analyze result
      let msgId = smsDatabaseService.saveSentMessage(number, message, timestamp);
      smsRequestManager.notifySmsSent(msgId, number, message, timestamp, requestId, processId);
    });
  }

Note how nsISmsDatabaseService::saveSentMessage() is called synchronously. This makes it pretty much impossible to implement with IndexedDB, for instance (modulo evil and r-able offences such as event loop spinning).

The easiest solution would be to move the "sent" notification (SmsRequestManager::notifySmsSent()) into nsISmsDatabaseService::saveSentMessage(). That way this operation could be async and do said notification on its own time. One might argue that we're now blurring the line between nsISmsService and nsISmsDatabaseService. This may be true, although given that SmsRequestManager::notifySmsSent() wants a message ID as input, which is determined by the database, we might as well accept the blurring already.

Another option would be to admit to the blurring and merge nsISmsService and nsISmsDatabaseService. This would not prevent us from deferring to more specialized components (RIL, IndexedDB-based storage) further down, but it would mean that we don't have to anticipate what sort of APIs various backends need between the sending/receiving part and the storage part. (Another example is storing *incoming* SMS, for which nsISmsDatabaseService anticipates no API right now. This is probably due to the fact that Android does this under the covers already, but for the RIL backend we'll need to plumb that through as an API... Or, you know, just merge the two objects and make it an implementation detail, just like on Android.)

Comment 1

[Mounir Lamouri (:mounir)]

I would prefer to keep the two services separate and saveSentMessage() shouldn't notify that the message has been sent, that would be a very poor API because highly not clear. If that was easy to do with JNI, I would have say that we could pass a callback in saveSentMessage() so things would be fully async but I would bet it's a hell if even doable to do with JNI.

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> I would prefer to keep the two services separate and saveSentMessage()
> shouldn't notify that the message has been sent, that would be a very poor
> API because highly not clear.

Sure, it wouldn't be the most straight-forward API, but such is often the nature of asynchronous programming. So far you're essentially saying no to the two options I could come up with. I'd like us to try and find a solution that will work for everybody. This may involve some compromises.

> If that was easy to do with JNI, I would have
> say that we could pass a callback in saveSentMessage() so things would be
> fully async but I would bet it's a hell if even doable to do with JNI.

What if SmsDatabaseService called a specific method on SmsService? Like this:

(1) SmsService::send(...) -> SmsDatabaseService::saveSentMessage(requestId, ...)
(2) SmsDatabaseService::saveSentMessage() -> SmsService::sentMessageSaved(requestId, successCode, message);
(4) SmsService::sentMessageSaved() -> SmsRequestManager::notifySmsSent(requestId, message);

This would put SmsService in charge of notifying SmsRequestManager, while SmsDatabaseService can do its operation asynchronously. Would that work on Android?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Philipp von Weitershausen [:philikon] from comment #0)
> There are some leaky abstractions in the interplay between
> nsISmsService::send() and nsISmsDatabaseService::saveSentMessage(). In
> particular, the interfaces assume that an nsISmsService::send()
> implementation will look something like this:
> 
>   function send(number, message, requestId, processId) {
>     doSendTextMessage(number, message, function (result) {
>       // ... analyze result
>       let msgId = smsDatabaseService.saveSentMessage(number, message,
> timestamp);
>       smsRequestManager.notifySmsSent(msgId, number, message, timestamp,
> requestId, processId);
>     });
>   }
> 
> Note how nsISmsDatabaseService::saveSentMessage() is called synchronously.
> This makes it pretty much impossible to implement with IndexedDB, for
> instance (modulo evil and r-able offences such as event loop spinning).
> 

What should happen if the save-to-DB fails?  Could that ever really happen in practice except in crazy edge cases like out of disk space?  If it does happen, what do we expect SMS apps to do about it?  The message was already sent, so resending just to get a copy in the DB seems a little silly.

If we can mostly ignore save-to-DB fails, then couldn't we implement saveSentMessage() by requesting an IDB transaction to do the write to disk?

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> What should happen if the save-to-DB fails?  Could that ever really happen
> in practice except in crazy edge cases like out of disk space?  If it does
> happen, what do we expect SMS apps to do about it?  The message was already
> sent, so resending just to get a copy in the DB seems a little silly.

Sure, but I still feel like we should bubble an error to that effect up to the DOM.

> If we can mostly ignore save-to-DB fails, then couldn't we implement
> saveSentMessage() by requesting an IDB transaction to do the write to disk?

Well, right now it's supposed to return the ID of the new object synchronously. How can you find out which ID to give it without doing a DB call? (Unless it's a randomly generated big ass number.) Even if we did the saving asynchronously, without success/failure reporting, we'd still have to deal with synchronously assigning IDs...

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

As I recall, the android backend doesn't generate a notification on fail-to-save-to-DB.  It assigns request IDs sequentially from a 64-bit counter.  The request IDs are transient since (again IIRC) they only refer to transient state dealing with particular API requests, not any data that persists.

If failing to save to DB isn't something that we in gecko or app developers can really do anything about, spec'ing it as infallible seems OK to me.  We can bolt error notifications on down the road if circumstances change.

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> As I recall, the android backend doesn't generate a notification on
> fail-to-save-to-DB.  It assigns request IDs sequentially from a 64-bit
> counter.  The request IDs are transient since (again IIRC) they only refer
> to transient state dealing with particular API requests, not any data that
> persists.

requestId != message.id. Pretty sure message.id is persisted and needs to be unique.

> If failing to save to DB isn't something that we in gecko or app developers
> can really do anything about, spec'ing it as infallible seems OK to me.  We
> can bolt error notifications on down the road if circumstances change.

I have no problem with that.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Gotcha, I was thinking about the wrong ID.  The relevant code from the android content DB is [1].

Turns out that insert() is synchronous and hits disk.  Ouch.  Reviewer fail (--> /me).  So yeah, we need to fix the DB-saving part of this.

Also, because I had the wrong request ID in mind, it means that the "sent SMS" notification is gated on save-to-disk, so there's no way to distinguish failed send from failed save-to-disk.  An argument could be made for having a separate "SMS made it to the network" notification, since it could allow updating UI sooner than the full save-to-disk.

[1] http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSmsManager.java#592
[2] http://developer.android.com/reference/android/content/ContentResolver.html#insert%28android.net.Uri,%20android.content.ContentValues%29

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Turns out that insert() is synchronous and hits disk.  Ouch.  Reviewer fail
> (--> /me).  So yeah, we need to fix the DB-saving part of this.

:)

> Also, because I had the wrong request ID in mind, it means that the "sent
> SMS" notification is gated on save-to-disk, so there's no way to distinguish
> failed send from failed save-to-disk.

Right. That was sort of the point I was trying to make in comment 0. You're putting it much more clearly, though.

> An argument could be made for having
> a separate "SMS made it to the network" notification, since it could allow
> updating UI sooner than the full save-to-disk.

I would not be opposed to this. This would be a fancier version of what I suggest in comment 2. Fancier in that we'd loop in SmsRequestManager between step (2) and (4) (look at that, as if I had anticipated there'd be a step (3) in the middle there!) which would notify the DOM of "sms-sent" and then later we'd notify "sms-saved" or whatever.

Comment 9

[Philipp von Weitershausen [:philikon]]

It seems the consensus is that we don't care about whether the message was successfully saved or not. If it was sent successfully, we should notify success. In particular, on Android, we should get the synchronous I/O out of the SMS send call path.

Morphing this bug to be about fixing the Android backend in that respect.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> It seems the consensus is that we don't care about whether the message was
> successfully saved or not.

I haven't seen any consensus here about that...

And we *have* to give a valid SmsMessage object when the sent event is fired. For that, we need a valid message's id.

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10)
> (In reply to Philipp von Weitershausen [:philikon] from comment #9)
> > It seems the consensus is that we don't care about whether the message was
> > successfully saved or not.
> 
> I haven't seen any consensus here about that...

Chris suggested in comment 5 and IRL discussions with ferjm, bent, and vicamo led to the same conclusion.

> And we *have* to give a valid SmsMessage object when the sent event is
> fired. For that, we need a valid message's id.

Yes. We can work around this for now in the RIL backend... If making the I/O non-blocking on Android requires changing nsISmsDatbaseService to have the ID notified later, we can always change the interface. My main intention was to unblock the SMS DB work for the RIL backend.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10)
> > (In reply to Philipp von Weitershausen [:philikon] from comment #9)
> > > It seems the consensus is that we don't care about whether the message was
> > > successfully saved or not.
> > 
> > I haven't seen any consensus here about that...
> 
> Chris suggested in comment 5 and IRL discussions with ferjm, bent, and
> vicamo led to the same conclusion.

If I may, it's quite easy to get a consensus if you make sure that the persons who do not agree are out of the discussion...

> > And we *have* to give a valid SmsMessage object when the sent event is
> > fired. For that, we need a valid message's id.
> 
> Yes. We can work around this for now in the RIL backend... If making the I/O
> non-blocking on Android requires changing nsISmsDatbaseService to have the
> ID notified later, we can always change the interface. My main intention was
> to unblock the SMS DB work for the RIL backend.

The only thing I want is to have the sent event being sent with a valid id and I do not want the Android backend to be put behind because the RIL backend needs to go forward. Otherwise, we can also just remove it from m-c...

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12)
> If I may, it's quite easy to get a consensus if you make sure that the
> persons who do not agree are out of the discussion...

My apologies, it was not our intent to cut you out of any discussion. After all, it's been almost a month since Chris wrote that comment, and in the spirit of moving forward, silence is often taken as consent here.

> > Yes. We can work around this for now in the RIL backend... If making the I/O
> > non-blocking on Android requires changing nsISmsDatbaseService to have the
> > ID notified later, we can always change the interface. My main intention was
> > to unblock the SMS DB work for the RIL backend.
> 
> The only thing I want is to have the sent event being sent with a valid id
> and I do not want the Android backend to be put behind because the RIL
> backend needs to go forward. Otherwise, we can also just remove it from
> m-c...

Nobody is suggested doing that. All I was trying to say in comment 9 and comment 11 was that we're going to punt on changing the nsISmsDatabaseService interface for now. In the gonk/RIL backed we're going to make the I/O async but the ID generation synchronous (for now). I personally think the Android backend should at least mimic the same behaviour, but perhaps it should go async all the way. In that case interface changes are needed, of course. You know the system much better, so you should make that call. My only goal was to unblock the gonk/RIL backend from this decision.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> Nobody is suggested doing that. All I was trying to say in comment 9 and
> comment 11 was that we're going to punt on changing the
> nsISmsDatabaseService interface for now. In the gonk/RIL backed we're going
> to make the I/O async but the ID generation synchronous (for now). I
> personally think the Android backend should at least mimic the same
> behaviour, but perhaps it should go async all the way. In that case
> interface changes are needed, of course. You know the system much better, so
> you should make that call. My only goal was to unblock the gonk/RIL backend
> from this decision.

The only requirement I have is that the sent event has a correctly constructed message. I think it's far more important than this being sync or async. If the message not being saved correctly doesn't seem like a problem, we can save the message later. Though, we still need the id.
Also, if the message is saved later, we have to support this use case:
sms.addEventListener('sent', function(e) {
  var req = sms.get(e.message.id);
  // this should not tell me the message isn't present in the db
});

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #14)
> The only requirement I have is that the sent event has a correctly
> constructed message. I think it's far more important than this being sync or
> async. If the message not being saved correctly doesn't seem like a problem,
> we can save the message later. Though, we still need the id.

Yes. That's how we're going to do this in the RIL/gonk/IndexedDB backend now: assign an ID + return early, save in the background.

> Also, if the message is saved later, we have to support this use case:
> sms.addEventListener('sent', function(e) {
>   var req = sms.get(e.message.id);
>   // this should not tell me the message isn't present in the db
> });

Good point. AFAIK this should work magically for our IndexedDB implementation because we serialize access to the DB.