Implement timeouts for IQ requests.

Implement timeouts for IQ requests.

Now the IqRequest class supports setting timeouts for each request, and 
JingleSession uses it to disconnect if no response is receive within 10 
secons from a request.

BUG=107925
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123680

[simonmorris, 2012-02-24 00:11:31]

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.cc:114: 
This is fragile if the host can send multiple requests to the same client
without waiting for a response after each request: a response could then be
matched to the wrong request.

Could this ever happen? If so, do responses contain any attributes that could
link them to a specific request, and is it worth checking first for a strong
match, and then falling back to any weaker matches?

Even if not, I think this logic is worth unit testing.

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:82: const std::string& jid);
|jid| is called "addressee" elsewhere. I'd suggest "toJid", but I don't mind as
long as it's consistent.

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:279: }
Log an error if request.get() is NULL?

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:510: // sent before it.
Why delete all requests sent before the given request?

[Sergey Ulanov, 2012-02-24 21:56:31]

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.cc:114: 
On 2012/02/24 00:11:31, simonmorris wrote:
> This is fragile if the host can send multiple requests to the same client
> without waiting for a response after each request: a response could then be
> matched to the wrong request.
>
> Could this ever happen? If so, do responses contain any attributes that could
> link them to a specific request, and is it worth checking first for a strong
> match, and then falling back to any weaker matches?

I believe with the current code it will not happen because we generate IQ ids
sequentially and map<> use binary search trees (i.e. we iterate over the
requests in the same order in which they were sent), but it is obviously
fragile. In any case, the way we use responses it doesn't really matter if
response to request A is interpreted as if it was for B and response to request
B is interpreted as if it was for A. What matters is that number of response is
the same as number of requests.  Also we almost never should have more than one
outstanding request.

Normally result stanzas don't have any useful information inside. Error stanzas
do, but using that information properly would make this code significantly more
complicated and wouldn't solve the problem anyway.

> 
> Even if not, I think this logic is worth unit testing.

Done

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:82: const std::string& jid);
On 2012/02/24 00:11:31, simonmorris wrote:
> |jid| is called "addressee" elsewhere. I'd suggest "toJid", but I don't mind
as
> long as it's consistent.

Done.

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:279: }
On 2012/02/24 00:11:31, simonmorris wrote:
> Log an error if request.get() is NULL?

Done.

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:510: // sent before it.
On 2012/02/24 00:11:31, simonmorris wrote:
> Why delete all requests sent before the given request?

The logic here is that if we send messages A, B and C and then suddenly receive
response to C then it means that responses to A and B were somehow lost. I think
that may happen only when the client switches from one network to another.
Ignoring timeouts for A and B seems to be the right thing to do in this case.
Added more comments about it.

[simonmorris, 2012-02-24 22:32:50]

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.cc:114: 
On 2012/02/24 21:56:31, sergeyu wrote:
> On 2012/02/24 00:11:31, simonmorris wrote:
> > This is fragile if the host can send multiple requests to the same client
> > without waiting for a response after each request: a response could then be
> > matched to the wrong request.
> >
> > Could this ever happen? If so, do responses contain any attributes that
could
> > link them to a specific request, and is it worth checking first for a strong
> > match, and then falling back to any weaker matches?
> 
> I believe with the current code it will not happen because we generate IQ ids
> sequentially and map<> use binary search trees (i.e. we iterate over the
> requests in the same order in which they were sent), but it is obviously
> fragile. In any case, the way we use responses it doesn't really matter if
> response to request A is interpreted as if it was for B and response to
request
> B is interpreted as if it was for A. What matters is that number of response
is
> the same as number of requests.  Also we almost never should have more than
one
> outstanding request.
> 
> Normally result stanzas don't have any useful information inside. Error
stanzas
> do, but using that information properly would make this code significantly
more
> complicated and wouldn't solve the problem anyway.
> 
> > 
> > Even if not, I think this logic is worth unit testing.
> 
> Done

OK

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:510: // sent before it.
On 2012/02/24 21:56:31, sergeyu wrote:
> On 2012/02/24 00:11:31, simonmorris wrote:
> > Why delete all requests sent before the given request?
> 
> The logic here is that if we send messages A, B and C and then suddenly
receive
> response to C then it means that responses to A and B were somehow lost. I
think
> that may happen only when the client switches from one network to another.
> Ignoring timeouts for A and B seems to be the right thing to do in this case.
> Added more comments about it.

The simple thing to do is just let A and B timeout as usual.  Under what
circumstances does that have a downside?

It's harder to reason about a system where there are three outcomes of sending a
message (response, timeout, ignore) instead of two (response, timeout). A future
developer could easily neglect the third case, especially if it happens
infrequently.

[Sergey Ulanov, 2012-02-25 00:11:52]

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:510: // sent before it.
On 2012/02/24 22:32:51, simonmorris wrote:
> On 2012/02/24 21:56:31, sergeyu wrote:
> > On 2012/02/24 00:11:31, simonmorris wrote:
> > > Why delete all requests sent before the given request?
> > 
> > The logic here is that if we send messages A, B and C and then suddenly
> receive
> > response to C then it means that responses to A and B were somehow lost. I
> think
> > that may happen only when the client switches from one network to another.
> > Ignoring timeouts for A and B seems to be the right thing to do in this
case.
> > Added more comments about it.
> 
> The simple thing to do is just let A and B timeout as usual.  Under what
> circumstances does that have a downside?

That would mean that the session will terminate even when we know that the peer
is still online. Not the best user experience.

> It's harder to reason about a system where there are three outcomes of sending
a
> message (response, timeout, ignore) instead of two (response, timeout). A
future
> developer could easily neglect the third case, especially if it happens
> infrequently.

That's only for the code that use JingleSession::SendMessage() and it uses the
same handler for all responses, so I don't think it makes big difference here.
In any case I added comments next to SendMessage() to explain its behavior.

[simonmorris, 2012-02-25 00:25:20]

LGTM

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/9452038/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:510: // sent before it.
On 2012/02/25 00:11:52, sergeyu wrote:
> On 2012/02/24 22:32:51, simonmorris wrote:
> > On 2012/02/24 21:56:31, sergeyu wrote:
> > > On 2012/02/24 00:11:31, simonmorris wrote:
> > > > Why delete all requests sent before the given request?
> > > 
> > > The logic here is that if we send messages A, B and C and then suddenly
> > receive
> > > response to C then it means that responses to A and B were somehow lost. I
> > think
> > > that may happen only when the client switches from one network to another.
> > > Ignoring timeouts for A and B seems to be the right thing to do in this
> case.
> > > Added more comments about it.
> > 
> > The simple thing to do is just let A and B timeout as usual.  Under what
> > circumstances does that have a downside?
> 
> That would mean that the session will terminate even when we know that the
peer
> is still online. Not the best user experience.
>

OK - if it's clear that a session can be considered alive with a response to C,
but no responses to A and B, then this makes sense.

> > It's harder to reason about a system where there are three outcomes of
sending
> a
> > message (response, timeout, ignore) instead of two (response, timeout). A
> future
> > developer could easily neglect the third case, especially if it happens
> > infrequently.
> 
> That's only for the code that use JingleSession::SendMessage() and it uses the
> same handler for all responses, so I don't think it makes big difference here.
> In any case I added comments next to SendMessage() to explain its behavior.

OK.

[commit-bot: I haz the power, 2012-02-25 01:44:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9452038/10003

[commit-bot: I haz the power, 2012-02-25 03:32:48]

Change committed as 123635

[jamesr, 2012-02-25 03:52:30]

On 2012/02/25 03:32:48, I haz the power (commit-bot) wrote:
> Change committed as 123635

This patch broke every compile and neither of you were in IRC, so I reverted it
in r1236358

[commit-bot: I haz the power, 2012-02-25 22:50:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9452038/11003

[commit-bot: I haz the power, 2012-02-25 22:52:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9452038/11003

[commit-bot: I haz the power, 2012-02-26 00:28:09]

Change committed as 123680