Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2417913002:
Process incoming IQs in the same order that they were sent. (Closed)
|
Created:
4 years, 2 months ago by kelvinp Modified:
4 years, 2 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProcess incoming IQs in the same order that they were sent.
The signaling channel does not guarantee that the incoming IQs are
delivered in the order that it is sent.
This behavior leads to transient session setup failures. For instance,
a <transport-info> that is sent after a <session-info> message is
sometimes delivered to the client out of order, causing the client to
close the session due to an unexpected request.
This CL implements a OrderedMessageQueue which guarantees the ordering of
the incoming GET and SEt IQ by extracting sequencing information from the
id attribute of incoming IQs.
Committed: https://crrev.com/897dbcba07e098ea819ba291bed12ccbbf1a91b1
Cr-Commit-Position: refs/heads/master@{#426936}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Reviewer's feedback #
Total comments: 46
Patch Set 3 : Reviewer's feedback #
Total comments: 6
Patch Set 4 : Reviewer's feedback #Patch Set 5 : Rebase #
Messages
Total messages: 22 (9 generated)
PTAL
kelvinp@chromium.org changed reviewers: + sergeyu@chromium.org
The CQ bit was checked by kelvinp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
See my suggestions for some cleanups we can do to make this CL simpler, particularly to avoid templates. https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/jingle_se... File remoting/protocol/jingle_session_manager.cc (right): https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/jingle_se... remoting/protocol/jingle_session_manager.cc:145: JingleMessageReply(error).ToXml(original_stanza)); Here original_stanza is not owned, so it may be deleted. Previously OnIncomingMessage() was always calling SendReply() synchronously, so it was guaranteed that original_stanza is still valid here, but this is not the case anymore given that reply_callback can be called later. We don't really need to attach original message content to the response (XMPP spec says it's optional, see https://tools.ietf.org/html/rfc6120#section-8.3.2). I suggest removing this feature, then this code can be cleaned up. It won't be necessary to pass reply_callback to OnIncomingMessage(). Instead JingleSession will be able to call session_manager_->SendReply() directly. https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/ordered_m... File remoting/protocol/ordered_message_queue.h (right): https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/ordered_m... remoting/protocol/ordered_message_queue.h:60: template <typename T> I'd like to avoid making this a template, given that it's only used in one place. See my other comment - it will allow to queue JingleMessage only, without a separate structure to store reply_callback.
https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/jingle_se... File remoting/protocol/jingle_session_manager.cc (right): https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/jingle_se... remoting/protocol/jingle_session_manager.cc:145: JingleMessageReply(error).ToXml(original_stanza)); On 2016/10/14 18:36:54, Sergey Ulanov wrote: > Here original_stanza is not owned, so it may be deleted. Previously > OnIncomingMessage() was always calling SendReply() synchronously, so it was > guaranteed that original_stanza is still valid here, but this is not the case > anymore given that reply_callback can be called later. > > We don't really need to attach original message content to the response (XMPP > spec says it's optional, see https://tools.ietf.org/html/rfc6120#section-8.3.2). > > I suggest removing this feature, then this code can be cleaned up. It won't be > necessary to pass reply_callback to OnIncomingMessage(). Instead JingleSession > will be able to call session_manager_->SendReply() directly. Good point on the point validity. A couple notes, when sending message to a host that is offline, today the XMPP framework sends an <network-unavailable> error stanza that includes the original stanza. The bot uses the from-endpoint-id in the original stanza to route the error back to the LCS client. It would be weird to expect the original stanza in some cases but not others. JingleMessageReply.toXml still needs the original stanza to figure out the the ID, and the address to reply to. So I would suggest leaving this change as is and invoke the copy constructor to copy the stanza over. WDYT?
On 2016/10/14 22:49:18, kelvinp wrote: > https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/jingle_se... > File remoting/protocol/jingle_session_manager.cc (right): > > https://codereview.chromium.org/2417913002/diff/1/remoting/protocol/jingle_se... > remoting/protocol/jingle_session_manager.cc:145: > JingleMessageReply(error).ToXml(original_stanza)); > On 2016/10/14 18:36:54, Sergey Ulanov wrote: > > Here original_stanza is not owned, so it may be deleted. Previously > > OnIncomingMessage() was always calling SendReply() synchronously, so it was > > guaranteed that original_stanza is still valid here, but this is not the case > > anymore given that reply_callback can be called later. > > > > We don't really need to attach original message content to the response (XMPP > > spec says it's optional, see > https://tools.ietf.org/html/rfc6120#section-8.3.2). > > > > I suggest removing this feature, then this code can be cleaned up. It won't be > > necessary to pass reply_callback to OnIncomingMessage(). Instead JingleSession > > will be able to call session_manager_->SendReply() directly. > > Good point on the point validity. > > A couple notes, when sending message to a host that is offline, today the XMPP > framework sends an <network-unavailable> error stanza that includes the original > stanza. The bot uses the from-endpoint-id in the original stanza to route the > error back to the LCS client. It would be weird to expect the original stanza > in some cases but not others. > > JingleMessageReply.toXml still needs the original stanza to figure out the the > ID, and the address to reply to. > > So I would suggest leaving this change as is and invoke the copy constructor to > copy the stanza over. WDYT? I don't mind that, but I would still prefer to avoid templates in OrderedMessageQueue. Maybe nest it inside JingleSession where it can us QueueItem directly? (BTW I also suggest to rename QueueItem to PendingMessage or something like that). For unittests maybe add message reordering FakeSignalStrategy and add a test in jingle_session_unittests.cc.
Patchset #2 (id:20001) has been deleted
PTAL Removed the template parameter and uses a copy of the stanza in jingle session manager.
Looks mostly good. I have some suggestions on how the test could be simplified, and some style nits. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:75: int GetSequentialId(std::string id) { please use const reference for string parameters https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:76: int result = kInvalid; move this below, just before StringToInt() call. See https://google.github.io/styleguide/cppguide.html#Local_Variables https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:88: }; nit: don't need ; https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:108: next_outgoing_(0), Move these initializers to where they are defined. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:110: base::RandGenerator(std::numeric_limits<uint64_t>::max()))){}; don't need semicolon https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:110: base::RandGenerator(std::numeric_limits<uint64_t>::max()))){}; base::RandUint64() https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:115: std::list<std::unique_ptr<PendingMessage>> OnIncomingMessage( Better to use std::vector<> instead of std::list<> for the result. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:116: std::string id, const reference https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:121: // Implements an ordered list by using map with the |sequence_id| as the key, I don't think you really need a map. A heap stored in std::vector<> would be more appropriate (see std::push_heap and std::pop_heap), or it can be sorted vector (there will never be more than a handful of messages, so message insertion consts are small). https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:125: int next_incoming_; = kAny https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:126: int next_outgoing_; = 0 https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:153: << "Duplicate sequence id: current= " << current Messages in DCHECK() are very rarely useful, but they increase code complexity, so I'd prefer removing them https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:158: queue_.insert(std::pair<int, std::unique_ptr<JingleSession::PendingMessage>>( std::make_pair? https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:175: std::string JingleSession::OrderedMessageQueue::GetNextOutgoingId() { This doesn't look related to queuing incoming messages. Move it to JingleSession? https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:496: for (auto it = ordered.begin(); it != ordered.end(); it++) { for (auto& message : ordered) Also for iterators (++it is faster than it++) https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:742: JingleSession::PendingMessage::PendingMessage( nit: put this above JingleSession::JingleSession() constructor. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session.h (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.h:85: void OnIncomingMessage(std::string id, const reference please https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.h:91: void OnIncomingMessageInOrder(std::unique_ptr<JingleMessage> message, Call this ProcessIncomingMessage()? https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:68: class MockTransport : public Transport { Maybe rename this to FakeTransport and stop using GMock for this class. Instead the class could just store the messages it receives: bool ProcessTransportInfo(buzz::XmlElement* transport_info) { received_message_.push_back( base::MakeUnique<buzz::XmlElement>(transport_info)); } std::vector<std::unique_ptr<buzz::XmlElement>> received_messages() { return received_messages_; } private: std::vector<std::unique_ptr<buzz::XmlElement>> received_messages_; Then in the test: EXPECT_EQ(transport_->received_messages()[0]->Attr(buzz::QN_ID), "1"); https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:76: base::Passed(CreateTransportInfo("1")))); I think it's better to move this inside the test itself. Just store send_transport_info_callback here, and then add function for the tests to access it: SendTransportInfoCallback send_transport_info_callback() { return send_transport_info_callback_; } In the tests: transport->send_transport_info_callback().Run(CreateTransportInfo("0")); This would allow to send transport messages only for ConnectWithOutofOrderIqs test, where we need them. They are not useful in other tests. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:85: std::unique_ptr<buzz::XmlElement> CreateTransportInfo(std::string id) { const reference for the parameter. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/order... File remoting/protocol/ordered_message_queue.h (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/order... remoting/protocol/ordered_message_queue.h:5: #ifndef REMOTING_PROTOCOL_ORDERED_MESSAGE_QUEUE_H_ remove this file? https://codereview.chromium.org/2417913002/diff/40001/remoting/signaling/fake... File remoting/signaling/fake_signal_strategy.cc (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/signaling/fake... remoting/signaling/fake_signal_strategy.cc:154: if (action != "session-info" && action != "transport-info") { I don't think we really need this logic here. The test could work as follows: 1. Initialize connection. 2. Call SimulatePackgeReordering(). 3. Send a set of transport messages and verify that they are received in correct order.
PTAL https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:75: int GetSequentialId(std::string id) { On 2016/10/19 21:23:38, Sergey Ulanov wrote: > please use const reference for string parameters Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:76: int result = kInvalid; On 2016/10/19 21:23:38, Sergey Ulanov wrote: > move this below, just before StringToInt() call. See > https://google.github.io/styleguide/cppguide.html#Local_Variables Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:88: }; On 2016/10/19 21:23:38, Sergey Ulanov wrote: > nit: don't need ; Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:108: next_outgoing_(0), On 2016/10/19 21:23:38, Sergey Ulanov wrote: > Move these initializers to where they are defined. Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:110: base::RandGenerator(std::numeric_limits<uint64_t>::max()))){}; On 2016/10/19 21:23:38, Sergey Ulanov wrote: > base::RandUint64() Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:110: base::RandGenerator(std::numeric_limits<uint64_t>::max()))){}; On 2016/10/19 21:23:38, Sergey Ulanov wrote: > base::RandUint64() Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:115: std::list<std::unique_ptr<PendingMessage>> OnIncomingMessage( On 2016/10/19 21:23:38, Sergey Ulanov wrote: > Better to use std::vector<> instead of std::list<> for the result. Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:116: std::string id, On 2016/10/19 21:23:38, Sergey Ulanov wrote: > const reference Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:121: // Implements an ordered list by using map with the |sequence_id| as the key, On 2016/10/19 21:23:39, Sergey Ulanov wrote: > I don't think you really need a map. A heap stored in std::vector<> would be > more appropriate (see std::push_heap and std::pop_heap), or it can be sorted > vector (there will never be more than a handful of messages, so message > insertion consts are small). Looking at std::push_heap() this would require us to write a comparator and store the id in the structure. Alternatively, we can use a priority_queue which called make_heap for us. Given we only have a handful of messages, it won't make a big difference either way we go. I will leave the code as is as it is simpler. WDYT? https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:125: int next_incoming_; On 2016/10/19 21:23:38, Sergey Ulanov wrote: > = kAny Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:126: int next_outgoing_; On 2016/10/19 21:23:38, Sergey Ulanov wrote: > = 0 Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:153: << "Duplicate sequence id: current= " << current On 2016/10/19 21:23:38, Sergey Ulanov wrote: > Messages in DCHECK() are very rarely useful, but they increase code complexity, > so I'd prefer removing them Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:158: queue_.insert(std::pair<int, std::unique_ptr<JingleSession::PendingMessage>>( On 2016/10/19 21:23:38, Sergey Ulanov wrote: > std::make_pair? Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:175: std::string JingleSession::OrderedMessageQueue::GetNextOutgoingId() { On 2016/10/19 21:23:38, Sergey Ulanov wrote: > This doesn't look related to queuing incoming messages. Move it to > JingleSession? Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:496: for (auto it = ordered.begin(); it != ordered.end(); it++) { On 2016/10/19 21:23:38, Sergey Ulanov wrote: > for (auto& message : ordered) > > Also for iterators (++it is faster than it++) Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:742: JingleSession::PendingMessage::PendingMessage( On 2016/10/19 21:23:38, Sergey Ulanov wrote: > nit: put this above JingleSession::JingleSession() constructor. Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session.h (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.h:85: void OnIncomingMessage(std::string id, On 2016/10/19 21:23:39, Sergey Ulanov wrote: > const reference please Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.h:91: void OnIncomingMessageInOrder(std::unique_ptr<JingleMessage> message, On 2016/10/19 21:23:39, Sergey Ulanov wrote: > Call this ProcessIncomingMessage()? Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:68: class MockTransport : public Transport { On 2016/10/19 21:23:39, Sergey Ulanov wrote: > Maybe rename this to FakeTransport and stop using GMock for this class. Instead > the class could just store the messages it receives: > > bool ProcessTransportInfo(buzz::XmlElement* transport_info) { > received_message_.push_back( > base::MakeUnique<buzz::XmlElement>(transport_info)); > } > std::vector<std::unique_ptr<buzz::XmlElement>> received_messages() { > return received_messages_; > } > private: > std::vector<std::unique_ptr<buzz::XmlElement>> received_messages_; > > > Then in the test: > > EXPECT_EQ(transport_->received_messages()[0]->Attr(buzz::QN_ID), > "1"); Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:76: base::Passed(CreateTransportInfo("1")))); On 2016/10/19 21:23:39, Sergey Ulanov wrote: > I think it's better to move this inside the test itself. > Just store send_transport_info_callback here, and then add function for the > tests to access it: > SendTransportInfoCallback send_transport_info_callback() { > return send_transport_info_callback_; > } > > In the tests: > transport->send_transport_info_callback().Run(CreateTransportInfo("0")); > > > This would allow to send transport messages only for ConnectWithOutofOrderIqs > test, where we need them. They are not useful in other tests. Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:85: std::unique_ptr<buzz::XmlElement> CreateTransportInfo(std::string id) { On 2016/10/19 21:23:39, Sergey Ulanov wrote: > const reference for the parameter. Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/order... File remoting/protocol/ordered_message_queue.h (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/protocol/order... remoting/protocol/ordered_message_queue.h:5: #ifndef REMOTING_PROTOCOL_ORDERED_MESSAGE_QUEUE_H_ On 2016/10/19 21:23:39, Sergey Ulanov wrote: > remove this file? Done. https://codereview.chromium.org/2417913002/diff/40001/remoting/signaling/fake... File remoting/signaling/fake_signal_strategy.cc (right): https://codereview.chromium.org/2417913002/diff/40001/remoting/signaling/fake... remoting/signaling/fake_signal_strategy.cc:154: if (action != "session-info" && action != "transport-info") { On 2016/10/19 21:23:39, Sergey Ulanov wrote: > I don't think we really need this logic here. The test could work as follows: > 1. Initialize connection. > 2. Call SimulatePackgeReordering(). > 3. Send a set of transport messages and verify that they are received in > correct order. Done.
lgtm after my comments are addressed https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:141: DCHECK(current >= next_incoming_); DCHECK_GE https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:319: client_signal_strategy_->SimulatePackgeReordering(); Move this after InitiateConnection(). Then FakeSignalStrategy won't need to check action type. https://codereview.chromium.org/2417913002/diff/60001/remoting/signaling/fake... File remoting/signaling/fake_signal_strategy.cc (right): https://codereview.chromium.org/2417913002/diff/60001/remoting/signaling/fake... remoting/signaling/fake_signal_strategy.cc:154: if (action != "transport-info") { Updated the test to reorder messages only after handshake, then this won't be necessary.
FYI https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:141: DCHECK(current >= next_incoming_); On 2016/10/21 19:19:39, Sergey Ulanov wrote: > DCHECK_GE Done. https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... File remoting/protocol/jingle_session_unittest.cc (right): https://codereview.chromium.org/2417913002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_session_unittest.cc:319: client_signal_strategy_->SimulatePackgeReordering(); On 2016/10/21 19:19:39, Sergey Ulanov wrote: > Move this after InitiateConnection(). Then FakeSignalStrategy won't need to > check action type. Done. https://codereview.chromium.org/2417913002/diff/60001/remoting/signaling/fake... File remoting/signaling/fake_signal_strategy.cc (right): https://codereview.chromium.org/2417913002/diff/60001/remoting/signaling/fake... remoting/signaling/fake_signal_strategy.cc:154: if (action != "transport-info") { On 2016/10/21 19:19:39, Sergey Ulanov wrote: > Updated the test to reorder messages only after handshake, then this won't be > necessary. Done.
The CQ bit was checked by kelvinp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2417913002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Process incoming IQs in the same order that they were sent. The signaling channel does not guarantee that the incoming IQs are delivered in the order that it is sent. This behavior leads to transient session setup failures. For instance, a <transport-info> that is sent after a <session-info> message is sometimes delivered to the client out of order, causing the client to close the session due to an unexpected request. This CL implements a OrderedMessageQueue which guarantees the ordering of the incoming GET and SEt IQ by extracting sequencing information from the id attribute of incoming IQs. ========== to ========== Process incoming IQs in the same order that they were sent. The signaling channel does not guarantee that the incoming IQs are delivered in the order that it is sent. This behavior leads to transient session setup failures. For instance, a <transport-info> that is sent after a <session-info> message is sometimes delivered to the client out of order, causing the client to close the session due to an unexpected request. This CL implements a OrderedMessageQueue which guarantees the ordering of the incoming GET and SEt IQ by extracting sequencing information from the id attribute of incoming IQs. Committed: https://crrev.com/897dbcba07e098ea819ba291bed12ccbbf1a91b1 Cr-Commit-Position: refs/heads/master@{#426936} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/897dbcba07e098ea819ba291bed12ccbbf1a91b1 Cr-Commit-Position: refs/heads/master@{#426936} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
