Merge lp:~al-maisan/landscape-client/missing-context into lp:~landscape/landscape-client/trunk

[Muharem Hrnjadovic]

This branch introduces the following change: response messages to the server for which no message context could be found are considered obsolete and dropped (as opposed to being sent).

[Free Ekanayaka (free.ekanayaka)]

Thanks for looking into this Muharem.

This is a reasonable workaround, and probably a fix that should be landed regardless of the attached bug. However we should also try to understand what happened exactly that led to this situation. I'll try to do that as a brain break.

+1!

review: Approve

[Jamu Kakar (jkakar)]

This looks fine I think. I'm slightly concerned about it, because it
may hide bugs in the future, but I think it's probably okay, +1.

review: Approve

[Jamu Kakar (jkakar)]

I wonder if it'd be better to report back to the server that the
operation is not understandable by the client...? Or maybe even to
just report that the operation-id was discarded so that we have this
information and can monitor how often it happens.

[Muharem Hrnjadovic (al-maisan)]

On 10/21/2010 10:32 PM, Jamu Kakar wrote:
> I wonder if it'd be better to report back to the server that the
> operation is not understandable by the client...? Or maybe even to
> just report that the operation-id was discarded so that we have this
> information and can monitor how often it happens.
>
That's a good idea, I have filed bug
https://bugs.edge.launchpad.net/landscape-client/+bug/665005

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

=== modified file 'landscape/broker/exchange.py'
--- landscape/broker/exchange.py	2010-04-30 10:36:51 +0000
+++ landscape/broker/exchange.py	2010-10-20 14:34:40 +0000
@@ -23,7 +23,7 @@
 
     def __init__(self, reactor, store, transport, registration_info,
                  exchange_store,
-                 exchange_interval=60*60,
+                 exchange_interval=60 * 60,
                  urgent_exchange_interval=10,
                  monitor_interval=None,
                  max_messages=100,
@@ -80,7 +80,7 @@
             logging.warning(
                 "No message context for message with operation-id: %s"
                 % operation_id)
-            return False
+            return True
 
         # Compare the current secure ID with the one that was in effect when
         # the request message was received.
@@ -99,8 +99,7 @@
         """
         if self._message_is_obsolete(message):
             logging.info(
-                "Response message with operation-id %s was discarded "
-                "because the client's secure ID has changed in the meantime"
+                "Obsolete response message with operation-id %s was discarded."
                 % message.get('operation-id'))
             return None
 

=== modified file 'landscape/broker/tests/test_exchange.py'
--- landscape/broker/tests/test_exchange.py	2010-04-30 16:14:09 +0000
+++ landscape/broker/tests/test_exchange.py	2010-10-20 14:34:40 +0000
@@ -240,6 +240,7 @@
         self.mstore.add({"type": "data", "data": 2})
         self.mstore.add({"type": "data", "data": 3})
         # next one, server will respond with 1!
+
         def desynched_send_data(payload, computer_id=None, message_api=None):
             self.transport.next_expected_sequence = 1
             return {"next-expected-sequence": 1}
@@ -303,7 +304,7 @@
         self.wait_for_exchange(urgent=True)
         self.assertEquals(len(self.transport.payloads), 1)
         self.wait_for_exchange(urgent=True)
-        self.assertEquals(len(self.transport.payloads), 1) # no change
+        self.assertEquals(len(self.transport.payloads), 1)  # no change
 
     def test_ancient_causes_resynchronize(self):
         """
@@ -569,9 +570,9 @@
         # schedule a regular exchange.
         # Let's make sure that that *original* impending-exchange event has
         # been cancelled:
-        self.reactor.advance(60 * 60 # time till exchange
-                             - 10 # time till notification
-                             - 20) # time that we've already advanced
+        self.reactor.advance(60 * 60  # time till exchange
+                             - 10  # time till notification
+                             - 20)  # time that we've already advanced
         self.assertEquals(events, [True])
         # Ok, so no new events means that the original call was
         # cancelled. great.
@@ -846,8 +847,8 @@
         self.assertEquals([], messages)
         self.assertIs(None, message_id)
         expected_log_entry = (
-            "Response message with operation-id 234567 was discarded because "
-            "the client's secure ID has changed in the meantime")
+            "Obsolete response message with operation-id 234567 was "
+            "discarded.")
         self.assertTrue(expected_log_entry in self.logfile.getvalue())
 
         # The MessageContext was removed after utilisation.
@@ -855,6 +856,35 @@
         self.assertTrue(len(ids_after) == len(ids_before) - 1)
         self.assertFalse('234567' in ids_after)
 
+    def test_response_messages_without_context_are_discarded(self):
+        """
+        A response message for which no incoming message context could be
+        found will be discarded as opposed to being sent to the server.
+        """
+        # Receive the message below from the server.
+        msg = {"type": "type-R", "whatever": 5678, "operation-id": 234567}
+        server_message = [msg]
+        self.transport.responses.append(server_message)
+        self.exchanger.exchange()
+
+        # Remove the message context so that the response message gets
+        # discarded.
+        self.exchange_store.get_message_context(msg['operation-id']).remove()
+        self.assertFalse('234567' in self.exchange_store.all_operation_ids())
+
+        self.mstore.set_accepted_types(["resynchronize"])
+        message_id = self.exchanger.send(
+            {"type": "resynchronize", "operation-id": 234567})
+        self.exchanger.exchange()
+        self.assertEquals(2, len(self.transport.payloads))
+        messages = self.transport.payloads[1]["messages"]
+        self.assertEquals([], messages)
+        self.assertIs(None, message_id)
+        expected_log_entry = (
+            "Obsolete response message with operation-id 234567 was "
+            "discarded.")
+        self.assertTrue(expected_log_entry in self.logfile.getvalue())
+
 
 class AcceptedTypesMessageExchangeTest(LandscapeTest):