[GCM] Add basic collapse key support for upstream

[GCM] Add basic collapse key support for upstream

Messages are collapsed based on app + token as long as they have not
been sent over the wire yet. If messages are resent (because original send
failed for some reason) no collapsing is done at this point in time.

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

[Nicolas Zea, 2014-01-27 16:01:22]

PTAL

[fgorski, 2014-01-27 17:39:37]

Only one concern: Is it safe to overwrite when the message is awaiting ack?

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:718: &message.GetProtobuf())->from();
app_id is stored in category

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:236: std::string app_id =
data_message->from();
same here

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:245: << original_packet->persistent_id;
Is it possible that you are assigning a persistent ID of a message that has
already been sent and is awaiting ack from the server? Because that would cause
the data carried in the message to be lost.

Did you think about that?

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:406: std::string app_id =
data_message->from();
ditto: category

[Nicolas Zea, 2014-01-28 08:15:34]

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:718: &message.GetProtobuf())->from();
On 2014/01/27 17:39:38, Filip Gorski wrote:
> app_id is stored in category

from is supposed to store project id, and category stores package name, right?
Id seems a better key to use (and is also what the gcm_store_impl is using for
per-app message limits)

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:245: << original_packet->persistent_id;
On 2014/01/27 17:39:38, Filip Gorski wrote:
> Is it possible that you are assigning a persistent ID of a message that has
> already been sent and is awaiting ack from the server? Because that would
cause
> the data carried in the message to be lost.
> 
> Did you think about that?

SendMessage is only invoked for new messages, not for resending. As soon as a
message is sent over the wire (in MaybeSendMessage), we remove it's key from the
collapse key map, whether it gets sent sucessfully/acknowledged or not.

[fgorski, 2014-01-28 18:10:27]

Clarification and more questions.

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:718: &message.GetProtobuf())->from();
On 2014/01/28 08:15:35, Nicolas Zea wrote:
> On 2014/01/27 17:39:38, Filip Gorski wrote:
> > app_id is stored in category
> 
> from is supposed to store project id, and category stores package name, right?
> Id seems a better key to use (and is also what the gcm_store_impl is using for
> per-app message limits)

Ok. For upstream messages the following holds true:
* to stores project Id, and that is what you want to use,
* from is set to an arbitrary string ("gcm@chrome.com"),
* category indeed stores app_id/package name.

From would be set to sender ID/project Id on downstream messages.

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:245: << original_packet->persistent_id;
On 2014/01/28 08:15:35, Nicolas Zea wrote:
> On 2014/01/27 17:39:38, Filip Gorski wrote:
> > Is it possible that you are assigning a persistent ID of a message that has
> > already been sent and is awaiting ack from the server? Because that would
> cause
> > the data carried in the message to be lost.
> > 
> > Did you think about that?
> 
> SendMessage is only invoked for new messages, not for resending. As soon as a
> message is sent over the wire (in MaybeSendMessage), we remove it's key from
the
> collapse key map, whether it gets sent sucessfully/acknowledged or not.

Does that mean we can fill RMQ for the application with collapse key messages
using the same value of the key?

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:132: typedef std::string AppId;
If you mean to use project_id as a value of that field, calling it app_id might
be problematic.
* First we already use that name higher up the stack. (later in the code you are
getting the value of from (meaning project_id) and assign it to variable name
app_id)
* Second, there might be multiple apps using the same project ID to access
information.

Perhaps a combination of project_id/app_id (package name) works better?

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:783:
mcs_client()->SendMessage(message);
Speaking to the earlier comment. If connection breaks (assuming client was
logged in, and MaybeSendMessage was fired) after sending this, the message is
still in storage, with a collapse_key = "token", but we cannot overwrite it with
the next one, as it is no longer in the map. Is that correct?

[fgorski, 2014-01-28 18:13:31]

One more thing: multi-profile.

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:132: typedef std::string AppId;
On 2014/01/28 18:10:27, Filip Gorski wrote:
> If you mean to use project_id as a value of that field, calling it app_id
might
> be problematic.
> * First we already use that name higher up the stack. (later in the code you
are
> getting the value of from (meaning project_id) and assign it to variable name
> app_id)
> * Second, there might be multiple apps using the same project ID to access
> information.
> 
> Perhaps a combination of project_id/app_id (package name) works better?

And speaking of multi-profile scenario... what if multiple profiles have the
same app running?

[Nicolas Zea, 2014-01-31 11:58:42]

Comments addressed, PTAL

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:718: &message.GetProtobuf())->from();
On 2014/01/28 18:10:27, Filip Gorski wrote:
> On 2014/01/28 08:15:35, Nicolas Zea wrote:
> > On 2014/01/27 17:39:38, Filip Gorski wrote:
> > > app_id is stored in category
> > 
> > from is supposed to store project id, and category stores package name,
right?
> > Id seems a better key to use (and is also what the gcm_store_impl is using
for
> > per-app message limits)
> 
> Ok. For upstream messages the following holds true:
> * to stores project Id, and that is what you want to use,
> * from is set to an arbitrary string ("gcm@chrome.com"),
> * category indeed stores app_id/package name.
> 
> From would be set to sender ID/project Id on downstream messages.

Done.

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:236: std::string app_id =
data_message->from();
On 2014/01/27 17:39:38, Filip Gorski wrote:
> same here

Done.

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:245: << original_packet->persistent_id;
On 2014/01/28 18:10:27, Filip Gorski wrote:
> On 2014/01/28 08:15:35, Nicolas Zea wrote:
> > On 2014/01/27 17:39:38, Filip Gorski wrote:
> > > Is it possible that you are assigning a persistent ID of a message that
has
> > > already been sent and is awaiting ack from the server? Because that would
> > cause
> > > the data carried in the message to be lost.
> > > 
> > > Did you think about that?
> > 
> > SendMessage is only invoked for new messages, not for resending. As soon as
a
> > message is sent over the wire (in MaybeSendMessage), we remove it's key from
> the
> > collapse key map, whether it gets sent sucessfully/acknowledged or not.
> 
> Does that mean we can fill RMQ for the application with collapse key messages
> using the same value of the key?

Currently yes, if all the messages manage to send, but are never acknowledged by
the server. Otherwise we'll have an unbounded set of messages that are pending
acknowledgement.

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:406: std::string app_id =
data_message->from();
On 2014/01/27 17:39:38, Filip Gorski wrote:
> ditto: category

Done.

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:132: typedef std::string AppId;
On 2014/01/28 18:13:31, Filip Gorski wrote:
> On 2014/01/28 18:10:27, Filip Gorski wrote:
> > If you mean to use project_id as a value of that field, calling it app_id
> might
> > be problematic.
> > * First we already use that name higher up the stack. (later in the code you
> are
> > getting the value of from (meaning project_id) and assign it to variable
name
> > app_id)
> > * Second, there might be multiple apps using the same project ID to access
> > information.
> > 
> > Perhaps a combination of project_id/app_id (package name) works better?
> 
> And speaking of multi-profile scenario... what if multiple profiles have the
> same app running?

I've gone ahead and modified this to use a CollapseKey class that cleans this up
(and supports multi-profile better, good catch on that).

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

https://codereview.chromium.org/148293002/diff/50001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:783:
mcs_client()->SendMessage(message);
On 2014/01/28 18:10:27, Filip Gorski wrote:
> Speaking to the earlier comment. If connection breaks (assuming client was
> logged in, and MaybeSendMessage was fired) after sending this, the message is
> still in storage, with a collapse_key = "token", but we cannot overwrite it
with
> the next one, as it is no longer in the map. Is that correct?

Correct, they're treated as completely independent messages. As far as I
understand they need to be acknowledged separately (they have different
persistent id and stream id after all).

[fgorski, 2014-02-04 22:22:35]

lgtm

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:722: // TODO(zea); consider verifying
the specific message already exists.
s/;/:/

[jianli, 2014-02-04 22:55:12]

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

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:98: bool CollapseKey::operator<(const
CollapseKey& right) const {
nit: empty line

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:273: // same collapsed key.
nit: collapse

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:39: class CollapseKey;
nit: sort

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:223: std::map<CollapseKey,
ReliablePacketInfo*> collapse_key_map_;
It seems that the value "ReliablePacketInfo*" is owned by to_send_ and
to_resend_. If the packet is removed from to_send_ and we forget to update
collapse_key_map_, we will get the crash. This might happen when we call
SendMessage and then ResetStateAndBuildLoginRequest.

[Nicolas Zea, 2014-02-12 23:35:44]

Comments addressed, PTAL

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:722: // TODO(zea); consider verifying
the specific message already exists.
On 2014/02/04 22:22:36, Filip Gorski wrote:
> s/;/:/

Done.

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

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:98: bool CollapseKey::operator<(const
CollapseKey& right) const {
On 2014/02/04 22:55:14, jianli wrote:
> nit: empty line

Done.

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:273: // same collapsed key.
On 2014/02/04 22:55:14, jianli wrote:
> nit: collapse

Done.

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:39: class CollapseKey;
On 2014/02/04 22:55:14, jianli wrote:
> nit: sort

Done.

https://codereview.chromium.org/148293002/diff/130001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:223: std::map<CollapseKey,
ReliablePacketInfo*> collapse_key_map_;
On 2014/02/04 22:55:14, jianli wrote:
> It seems that the value "ReliablePacketInfo*" is owned by to_send_ and
> to_resend_. If the packet is removed from to_send_ and we forget to update
> collapse_key_map_, we will get the crash. This might happen when we call
> SendMessage and then ResetStateAndBuildLoginRequest.

Good catch. Note that to_resend_ will never share packets with the
collapse_key_map_, only to_send_. I've gone ahead and added the
PopMessageForSend helper to ensure the collapse key map is always updated if
necessary.

[jianli, 2014-02-13 00:01:17]

lgtm

[Nicolas Zea, 2014-02-13 18:04:01]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:06:26]

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

[commit-bot: I haz the power, 2014-02-13 19:55:14]

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

[commit-bot: I haz the power, 2014-02-13 21:42:36]

Change committed as 251135