[GCM] Add TTL support

[GCM] Add TTL support

TTL is currently enforced at Send time, resend time, and reconnect. It
does not take into account clock changes.

BUG=284553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242956

[Nicolas Zea, 2013-12-28 00:49:39]

PTAL. A follow up patch will make better use of the message sent callback, for
now the contents aren't that useful.

[fgorski, 2013-12-28 01:15:08]

Hey a few comments, I'll take another look after New Years.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:214: return;
should be indented 2 to the left (under reinterpret)
applies below as well.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:243: if (ttl > 0 &&
would a single return in place of if statement work?

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:255: return reinterpret_cast<const
mcs_proto::DataMessageStanza*>(&protobuf)->
ttl might not be set, return max in that case.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:225: DCHECK_LT(ttl, kMaxTTLSeconds);
DCHECK_LE

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:323: message_sent_callback_.Run("TTL
expired for " + packet->persistent_id);
This message is going to be a bit pointless when persistent_id is empty.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:105: // OnMessageSent is invoked with a TTL
expiration error.
do you mean message_sent_callback_?

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:281:
EXPECT_TRUE(GetFakeHandler()->
one line

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:604:
EXPECT_TRUE(GetFakeHandler()->
one line. fix other places.

[Nicolas Zea, 2013-12-30 21:46:18]

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:214: return;
On 2013/12/28 01:15:08, Filip Gorski wrote:
> should be indented 2 to the left (under reinterpret)
> applies below as well.

Done.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:243: if (ttl > 0 &&
On 2013/12/28 01:15:08, Filip Gorski wrote:
> would a single return in place of if statement work?

Done.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:255: return reinterpret_cast<const
mcs_proto::DataMessageStanza*>(&protobuf)->
On 2013/12/28 01:15:08, Filip Gorski wrote:
> ttl might not be set, return max in that case.

Should we default to max, or assume that the message is not reliable? It seems a
bit safer to assume take the less expensive option.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:225: DCHECK_LT(ttl, kMaxTTLSeconds);
On 2013/12/28 01:15:08, Filip Gorski wrote:
> DCHECK_LE

Done.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:323: message_sent_callback_.Run("TTL
expired for " + packet->persistent_id);
On 2013/12/28 01:15:08, Filip Gorski wrote:
> This message is going to be a bit pointless when persistent_id is empty.

Yeah, all of these messages are pointless right now. Changed to just "TTL
expired".

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:105: // OnMessageSent is invoked with a TTL
expiration error.
On 2013/12/28 01:15:08, Filip Gorski wrote:
> do you mean message_sent_callback_?

Done.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:281:
EXPECT_TRUE(GetFakeHandler()->
On 2013/12/28 01:15:08, Filip Gorski wrote:
> one line

Done.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:604:
EXPECT_TRUE(GetFakeHandler()->
On 2013/12/28 01:15:08, Filip Gorski wrote:
> one line. fix other places.

Done.

[fgorski, 2014-01-02 18:09:26]

LGTM provided you agree with me on defaulting TTL to max, otherwise let's have a
chat.

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:255: return reinterpret_cast<const
mcs_proto::DataMessageStanza*>(&protobuf)->
On 2013/12/30 21:46:19, Nicolas Zea wrote:
> On 2013/12/28 01:15:08, Filip Gorski wrote:
> > ttl might not be set, return max in that case.
> 
> Should we default to max, or assume that the message is not reliable? It seems
a
> bit safer to assume take the less expensive option.

The API allows the user to not set the TTL and a default equal to MAX is assumed
in that case. We can compensate for that and explicitly set it on the message at
some point, but we would have to be really smart about it.
If you omit setting TTL on protobuf, MCS will assume max.
I think so should the client side code in case of missing TTL.

[jianli, 2014-01-02 18:52:58]

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/base/mc...
File google_apis/gcm/base/mcs_util.h (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/base/mc...
google_apis/gcm/base/mcs_util.h:86: GCM_EXPORT bool HasTTLExpired(const
google::protobuf::MessageLite& protobuf,
This function returns false when TTL is 0. It will be better to comment on this.

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:322:
dropped_ids.push_back(packet->persistent_id);
If TTL is 0, we do not give a persisten ID as in SendMessage. It seems that
dropped_ids will not be filled with TTL==0 message.

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:282: 
nit: empty line not needed

[Nicolas Zea, 2014-01-02 21:39:07]

PTAL

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/117513004/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:255: return reinterpret_cast<const
mcs_proto::DataMessageStanza*>(&protobuf)->
On 2014/01/02 18:09:27, Filip Gorski wrote:
> On 2013/12/30 21:46:19, Nicolas Zea wrote:
> > On 2013/12/28 01:15:08, Filip Gorski wrote:
> > > ttl might not be set, return max in that case.
> > 
> > Should we default to max, or assume that the message is not reliable? It
seems
> a
> > bit safer to assume take the less expensive option.
> 
> The API allows the user to not set the TTL and a default equal to MAX is
assumed
> in that case. We can compensate for that and explicitly set it on the message
at
> some point, but we would have to be really smart about it.
> If you omit setting TTL on protobuf, MCS will assume max.
> I think so should the client side code in case of missing TTL.

Done.

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/base/mc...
File google_apis/gcm/base/mcs_util.h (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/base/mc...
google_apis/gcm/base/mcs_util.h:86: GCM_EXPORT bool HasTTLExpired(const
google::protobuf::MessageLite& protobuf,
On 2014/01/02 18:52:58, jianli wrote:
> This function returns false when TTL is 0. It will be better to comment on
this.

Done.

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:322:
dropped_ids.push_back(packet->persistent_id);
On 2014/01/02 18:52:58, jianli wrote:
> If TTL is 0, we do not give a persisten ID as in SendMessage. It seems that
> dropped_ids will not be filled with TTL==0 message.

Right, but those ids weren't stored in the RMQStore anyways, so it's okay to
skip them. dropped_ids is just for removing messages from the rmq store.

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client_unittest.cc:282: 
On 2014/01/02 18:52:58, jianli wrote:
> nit: empty line not needed

Done.

[fgorski, 2014-01-02 21:47:34]

The change might be going to far with TTL.

https://codereview.chromium.org/117513004/diff/160001/google_apis/gcm/base/mc...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/117513004/diff/160001/google_apis/gcm/base/mc...
google_apis/gcm/base/mcs_util.cc:259: data_message->ttl() : kMaxTTLSeconds;
I think sanitizing of the input is expected to happen in the layer above so
DCHECK for > kMaxTTLSeconds would work here.

Missing TTL is a perfectly valid scenario. TTL too large is an error.

Will you send the protobuf knowing that TTL is too large? Will that be accepted
by MCS?

[Nicolas Zea, 2014-01-02 21:50:52]

https://codereview.chromium.org/117513004/diff/160001/google_apis/gcm/base/mc...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/117513004/diff/160001/google_apis/gcm/base/mc...
google_apis/gcm/base/mcs_util.cc:259: data_message->ttl() : kMaxTTLSeconds;
On 2014/01/02 21:47:35, Filip Gorski wrote:
> I think sanitizing of the input is expected to happen in the layer above so
> DCHECK for > kMaxTTLSeconds would work here.
> 
> Missing TTL is a perfectly valid scenario. TTL too large is an error.
> 
> Will you send the protobuf knowing that TTL is too large? Will that be
accepted
> by MCS?

Yeah, I guess this implies it's valid, when it shouldn't be. I'll just change it
to a DCHECK.

[jianli, 2014-01-02 21:51:31]

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:322:
dropped_ids.push_back(packet->persistent_id);
On 2014/01/02 21:39:08, Nicolas Zea wrote:
> On 2014/01/02 18:52:58, jianli wrote:
> > If TTL is 0, we do not give a persisten ID as in SendMessage. It seems that
> > dropped_ids will not be filled with TTL==0 message.
> 
> Right, but those ids weren't stored in the RMQStore anyways, so it's okay to
> skip them. dropped_ids is just for removing messages from the rmq store.
> 

But the comment at line 311 says: Drop all TTL == 0 or ...

Also you have the check for "GetTTL(*packet->protobuf) > 0". All theses make the
reader think we're handling TTL == 0 case here.

[Nicolas Zea, 2014-01-02 21:56:13]

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/117513004/diff/110001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:322:
dropped_ids.push_back(packet->persistent_id);
On 2014/01/02 21:51:31, jianli wrote:
> On 2014/01/02 21:39:08, Nicolas Zea wrote:
> > On 2014/01/02 18:52:58, jianli wrote:
> > > If TTL is 0, we do not give a persisten ID as in SendMessage. It seems
that
> > > dropped_ids will not be filled with TTL==0 message.
> > 
> > Right, but those ids weren't stored in the RMQStore anyways, so it's okay to
> > skip them. dropped_ids is just for removing messages from the rmq store.
> > 
> 
> But the comment at line 311 says: Drop all TTL == 0 or ...
> 
> Also you have the check for "GetTTL(*packet->protobuf) > 0". All theses make
the
> reader think we're handling TTL == 0 case here.

We are handling the case by removing those messages from the send queue. But,
they don't need to be removed from the persistent storage because they were
never added there. Do you think the name dropped_ids should be different to make
that distinction clearer?

[jianli, 2014-01-02 22:02:04]

On Thu, Jan 2, 2014 at 1:56 PM, <zea@chromium.org> wrote:

>
> https://codereview.chromium.org/117513004/diff/110001/
> google_apis/gcm/engine/mcs_client.cc
> File google_apis/gcm/engine/mcs_client.cc (right):
>
> https://codereview.chromium.org/117513004/diff/110001/
> google_apis/gcm/engine/mcs_client.cc#newcode322
> google_apis/gcm/engine/mcs_client.cc:322:
> dropped_ids.push_back(packet->persistent_id);
> On 2014/01/02 21:51:31, jianli wrote:
>
>> On 2014/01/02 21:39:08, Nicolas Zea wrote:
>> > On 2014/01/02 18:52:58, jianli wrote:
>> > > If TTL is 0, we do not give a persisten ID as in SendMessage. It
>>
> seems that
>
>> > > dropped_ids will not be filled with TTL==0 message.
>> >
>> > Right, but those ids weren't stored in the RMQStore anyways, so it's
>>
> okay to
>
>> > skip them. dropped_ids is just for removing messages from the rmq
>>
> store.
>
>> >
>>
>
>  But the comment at line 311 says: Drop all TTL == 0 or ...
>>
>
>  Also you have the check for "GetTTL(*packet->protobuf) > 0". All
>>
> theses make the
>
>> reader think we're handling TTL == 0 case here.
>>
>
> We are handling the case by removing those messages from the send queue.
> But, they don't need to be removed from the persistent storage because
> they were never added there. Do you think the name dropped_ids should be
> different to make that distinction clearer?
>

Yes. Thanks.

>
> https://codereview.chromium.org/117513004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nicolas Zea, 2014-01-02 22:06:35]

Done, PTAL

[jianli, 2014-01-02 23:16:46]

lgtm

[fgorski, 2014-01-02 23:42:55]

lgtm

[commit-bot: I haz the power, 2014-01-03 17:55:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/117513004/200002

[commit-bot: I haz the power, 2014-01-03 18:41:15]

Retried try job too often on linux_rel for step(s) chromedriver2_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-03 19:22:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/117513004/630001

[commit-bot: I haz the power, 2014-01-03 19:29:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/117513004/700001

[commit-bot: I haz the power, 2014-01-03 23:13:01]

Change committed as 242956