Enpei Reporter
	Description • 11 years ago

DUT can still receive same SI ID created earlier than already in device one.

* Build Number                
Gaia:     9fb5802df60a9081846d704def01df814ed8fbd4
Gecko:    http://hg.mozilla.org/mozilla-central/rev/b6c29e434519
BuildID   20130901040215
Version   26.0a1

* Reproduce Steps
1. Send SI with SI ID:abc, create date 2013-09-03T17:35:33Z from NowSMS to DUT.
2. Send another SI with same ID, but create date 2013-09-03T10:35:33Z

* Expected Result
DUT won't receive 2nd older message.

* Actual Result
DUT still receive it.

* Occurrence rate
100%.

	Enpei Reporter
	Comment 1 • 11 years ago

User story define in bug 891248, comment 8.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 2 • 11 years ago

This patch is a follow up to bug 911247 and applies on top of the patch for that bug. This adds parsing the optional 'si-id' and 'created' fields and uses them to handle out-of-order delivery of messages. Old messages are automatically expired when replaced by newer ones or ignored if a new one has already arrived. The patch also contains unit-tests covering the additional functionality.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 3 • 11 years ago

Comment on attachment 811195 [details] [diff] [review]
[PATCH] Handle out-of-order message reception

Clearing the review as I'll be refreshing this to accomodate for changes in bug 911247.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 4 • 11 years ago

Refreshed patch, applies on top of attachment 812573 [details] [diff] [review].

	Gabriele Svelto [:gsvelto] Assignee
	Comment 5 • 11 years ago

Updated patch rebased on top of attachment 813010 [details] [diff] [review].

	Gabriele Svelto [:gsvelto] Assignee
	Comment 7 • 11 years ago

Rebased on top of attachment 814826 [details] [diff] [review]. This implements out-of-order message handling and replacement according to the 'si-id' (identification) and 'created' (creation time fields). The behavior is described here (paragraph 5.2.1 and 6.2 articles 2 and 3):

http://is.gd/JnXr1a

The complete flow is shown here (paragraph 6.2):

http://is.gd/3jJNcf

In a nutshell this inspect messages and checks if they have si-id and created fields. If they have both messages the time stored in the created field is used to establish if the message is new (no other messages have been received with that si-id, or only older messages) or if it's obsolete (we've already seen a message with the same si-id and it's creation time is newer). Depending on this condition previously received messages can be expired and newly received ones can be ignored if they're already obsolete.

Message ordering and replacement is done within the database logic to enforce the atomicity of the operation and avoid race conditions. Unit-tests are provided.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 8 • 11 years ago

Pointer to Github pull-request

	Julien Wajsberg [:julienw]
	Comment 9 • 11 years ago

Comment on attachment 814928 [details] [diff] [review]
[PATCH v4] Handle out-of-order message reception

Review of attachment 814928 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, a few comments still.

Not for this bug, because we run out of time, but I still don't like the very tightly coupled logic in MessageDB, I think we can do better. I'm thinking of an promise-like API like this:

DB.transaction().then(...).then(...).finish(...).

"then" callbacks would be called in "onsuccess", so won't end the transaction.
"finish" callbacks would be called in "oncomplete", so at the end of the transaction.

A .then callback could pass parameters to the next .then callback, in addition to the transaction object.

Thought I could share it with you now, so that you can think about it in background :)

For your follow-ups, please keep your existing commit and push on github a single new commit, + your full patch here if you want. Both are good for me, but the single commit makes it obvious to see what changed :)

Thanks !

::: apps/wappush/js/messagedb.js
@@ +87,5 @@
>     * @param {Function} error A callback invoked if an operation fails.
>     */
>    function mdb_put(message, success, error) {
>      mdb_open(function mdb_putCallback(db) {
> +      var state = { status: 'new' };

I don't get why you have an object and not just a string here ?

::: apps/wappush/js/parsed_message.js
@@ +118,5 @@
> +      }
> +
> +      // 'created' attribute, optional, date in ISO 8601 format
> +      if (indicationNode.hasAttribute('created')) {
> +        obj.created = Date.parse(indicationNode.getAttribute('created'));

note that Date.parse is more permissive that what your wap push spec says. Is it ok for you to accept time zones for example ?

Also, you should do : `if (isNaN(obj.create)) { obj.create = undefined; }` in case `Date.parse` returns NaN. (doing `Date.parse(...) || undefined` will be incorrect for the epoch time :-) )

Last but not least, Date.parse returns a timezone-dependant timestamp. That means that if the user changes his timezone between 2 message receptions, your comparison will be wrong. So it's maybe better to do `new Date(value).getTime()`. This still returns NaN for badly formed strings, so my previous comments still hold.

::: apps/wappush/js/wappush.js
@@ +139,5 @@
>      }
>  
>      message.save(
> +      function wpm_saveSuccess(status) {
> +        if ((status === 'new') || (status === 'updated')) {

better do an early return here, if the status is different of both values

@@ +163,5 @@
> +            self.close(/* background */ true);
> +          };
> +          req.onerror = function wpm_getAppError() {
> +            self.close(/* background */ true);
> +          };

seems to me this req is out of the if block

@@ +164,5 @@
> +          };
> +          req.onerror = function wpm_getAppError() {
> +            self.close(/* background */ true);
> +          };
> +        }

add .bind(this) at the end of the function (so: `message.save(function() { ... }.bind(this))`) and you can use `this` instead of `self` inside the function. Same for all nested functions of course.

Might also be a good idea to extract this function to a top level function.

::: apps/wappush/test/unit/mock_messagedb.js
@@ +37,5 @@
> +    } else {
> +      messages.push(message);
> +    }
> +
> +    success(status);

I think you're throwing too much logic here.

Instead, from the test, you should spy on this, and call `spy.yield(value)` with whatever value you want to test your code for. This has a lot of advantages over your code:

1 - it is really asynchronous-like, like the reality: the callback is called only after the code after the "MessageDB.put" call has run.

2 - you can test very easily different cases.

3 - you're not testing yor mock (which is what you do when you have too much logic in it)

@@ +51,4 @@
>        }
>      }
>  
> +    success(null);

check if you have the callback before calling it

::: apps/wappush/test/unit/parsed_message_test.js
@@ +58,5 @@
> +                 '            created="2013-09-03T10:35:33Z">' +
> +                 'check this out' +
> +                 '</indication>' +
> +                 '</si>'
> +      },

one case with a bad `created` attribute would be good too, Date.parse should throw or whatever in that case :)

@@ +104,5 @@
> +
> +    test('SI message with explicit si-id field', function() {
> +      var message = ParsedMessage.from(messages.si_id, timestamp);
> +
> +      assert.equal(message.id, 'gaia-test@mozilla.org');

please test href too here

	Gabriele Svelto [:gsvelto] Assignee
	Comment 10 • 11 years ago

Here's an updated patch with all your comments addressed (more below):

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Not for this bug, because we run out of time, but I still don't like the
> very tightly coupled logic in MessageDB, I think we can do better. I'm
> thinking of an promise-like API like this:
> 
> DB.transaction().then(...).then(...).finish(...).
> 
> "then" callbacks would be called in "onsuccess", so won't end the
> transaction.
> "finish" callbacks would be called in "oncomplete", so at the end of the
> transaction.
> 
> A .then callback could pass parameters to the next .then callback, in
> addition to the transaction object.
> 
> Thought I could share it with you now, so that you can think about it in
> background :)

Sounds like a good idea for a generalized IDB helper, I'll try to implement something like that in the future.

> I don't get why you have an object and not just a string here ?

I can't remember why I did it either... I replaced it with a string.

> note that Date.parse is more permissive that what your wap push spec says.
> Is it ok for you to accept time zones for example ?

Yeah because I'll never get one, we generate the date internally in that format so there's no need to check it:

http://dxr.mozilla.org/mozilla-central/source/dom/wappush/src/gonk/SiPduHelper.jsm#l35

> Last but not least, Date.parse returns a timezone-dependant timestamp. That
> means that if the user changes his timezone between 2 message receptions,
> your comparison will be wrong. So it's maybe better to do `new
> Date(value).getTime()`.

I just did that, nice catch!

> better do an early return here, if the status is different of both values

Done.

> add .bind(this) at the end of the function (so: `message.save(function() {
> ... }.bind(this))`) and you can use `this` instead of `self` inside the
> function. Same for all nested functions of course.

Done, it's a long chain of bind()s but if it's preferred to the 'var self = this' style I don't mind.

> I think you're throwing too much logic here. [snip]

Got rid of all that stuff and used spy.yield() instead as you suggested :)

> check if you have the callback before calling it

Done.

> one case with a bad `created` attribute would be good too, Date.parse should
> throw or whatever in that case :)

I didn't do it as it should not happen (and if it does then it's a Gecko issue).

> please test href too here

Done.

	Julien Wajsberg [:julienw]
	Comment 11 • 11 years ago

Comment on attachment 821079 [details] [diff] [review]
[PATCH v5] Handle out-of-order message reception

Review of attachment 821079 [details] [diff] [review]:
-----------------------------------------------------------------

Next time, please generate patches with 8 lines of context ;) |git format-patch -U8| works fine, but I couldn't find how to make it the default.

r=me with the comments adressed and provided a green Travis. But feel free to ask me another review if you don't feel confident about the tests.

::: apps/wappush/test/unit/wappush_test.js
@@ +173,5 @@
>        MockNavigatormozSetMessageHandler.mTrigger('wappush-received', message);
>      });
>  
> +    teardown(function() {
> +      MockMessageDB.clear();

I'd create a `mTeardown` function in MockMessageDB, that would call `clear`.

and `mTeardown` functions are called automatically by the mocks helper (when you use mocks that are handled by the mocks helper), so you don't even need to call it yourself :)

@@ +241,5 @@
> +      WapPushManager.init(done);
> +    });
> +
> +    suiteTeardown(function() {
> +      MockMessageDB.clear();

you don't need to do this if you create a mTeardown function in this mock.

@@ +244,5 @@
> +    suiteTeardown(function() {
> +      MockMessageDB.clear();
> +      MockNavigatormozApps.mTeardown();
> +      MockNavigatorSettings.mTeardown();
> +      MockNavigatormozSetMessageHandler.mTeardown();

The name of these functions should have been a good reminder: they're supposed to be called in a `teardown` function, so that every test start with a clean state.

@@ +249,5 @@
> +    });
> +
> +    test('the old message is expired', function() {
> +      var putSpy = this.sinon.spy(MockMessageDB, 'put');
> +      var retrieveSpy = this.sinon.spy(MockMessageDB, 'retrieve');

I'd put all the spies in the `setup` function, so that you don't clobber the various test functions, and make it clearer to read.

And I wouldn't use variables either, for example using `MockMessageDB.retrieve.yield(null)` works fine and is imo more explicit.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 12 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #11)
> Next time, please generate patches with 8 lines of context ;) |git
> format-patch -U8| works fine, but I couldn't find how to make it the default.

I just added these lines to my .gitconfig file and they seem to do the job:

[diff]
    context = 8

> r=me with the comments adressed and provided a green Travis. But feel free
> to ask me another review if you don't feel confident about the tests.

I've addressed all the changes you requested in the review and especially the unit-test so I think we're good. I have learned a few more things about how to use the mocks and Sinon in this bug :-)

The Travis run is here, I'll wait for it to be green before merging:

https://travis-ci.org/mozilla-b2g/gaia/builds/13042724

	Julien Wajsberg [:julienw]
	Comment 13 • 11 years ago

(In reply to Gabriele Svelto [:gsvelto] from comment #12)

> [diff]
>     context = 8

This works for diff but it didn't work for format-patch for me. But maybe they fixed it ?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 14 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #13)
> This works for diff but it didn't work for format-patch for me. But maybe
> they fixed it ?

I've got version 1.8.3.1 FYI.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 15 • 11 years ago

Pushed to master and v1.2

gaia/master 1b2f11a0ad9a0deca867547be1a8cf95e2757d4a
gaia/v1.2   14f3f4e7b65ff79ef131ce4d3a8a3e45cab2d9d9

	Enpei Reporter
	Comment 16 • 11 years ago

Verified on Buri
Gaia:     df049e3177ced0ca493ff0d192c65f18392bb462
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/93eafd042c1c
BuildID   20131031004003
Version   26.0