Make protocol util a singleton per thread #1743
Conversation
…nuxAgent into SingletonWireClient
…oved locks from Protocol util as there is an uber lock in the singleton class per thread and made protocol a private variable. Updating goalstate now in monitor thread to ensure its always uptodate Added relevant test cases
Codecov Report
@@ Coverage Diff @@
## develop #1743 +/- ##
===========================================
+ Coverage 68.5% 68.58% +0.07%
===========================================
Files 82 82
Lines 11737 11743 +6
Branches 1645 1646 +1
===========================================
+ Hits 8041 8054 +13
+ Misses 3350 3345 -5
+ Partials 346 344 -2
Continue to review full report at Codecov.
|
| monitor_handler.start() | ||
| time.sleep(1) | ||
| self.assertTrue(monitor_handler.is_alive()) | ||
| with patch.object(monitor_handler, 'protocol'): |
There was a problem hiding this comment.
What is this patching? Are you forcefully returning a mock when looking at self.protocol?
There was a problem hiding this comment.
Or you intend to test it with protocol to be None
There was a problem hiding this comment.
Yes, I'm forcefully returning a mock for the monitor_thread.protocol object. This was not required before my changes because we weren't calling the protocol.update_goal_state() before my changes. This change in behaviour was breaking the test due to which I had to mock it
| self.assertEqual(obj1.uuid, obj2.uuid) | ||
|
|
||
| def test_it_should_have_multiple_instances_for_multiple_threads(self): | ||
| output = Queue() |
There was a problem hiding this comment.
just curious: why output is a local and errors an instance member?
There was a problem hiding this comment.
Mainly because the error checking is done on a class level in the _setup_multithread_and_execute class but the checks for output are specific to the test.
| last_incarnation = None | ||
| if os.path.isfile(incarnation_file): | ||
| last_incarnation = fileutil.read_file(incarnation_file) | ||
| last_incarnation = self.get_goal_state().incarnation |
There was a problem hiding this comment.
should we do this until we remove the cache on disk? the current code always reads the file; get_goal_state may return the in-memory value
There was a problem hiding this comment.
I think we should keep this change on how we check for last_incarnation because without this the objects would again go out of sync as soon as some other thread updates the goal_state.
I can change the get_goal_state() to self.goal_state until the file cache is removed though.
…led in the next PR
Description
Recently we discovered a bug in the Guest Agent where the WireClient object different for different handlers which was due to the fact that a new ProtocolUtil object was being instantiated for every Hanlder (UpdateHandler, ExtHandler, etc).
This PR addresses this issue by making sure that a single ProtocolUtil object is used for each thread. It was made a SingletonPerThread to avoid the concurrency issues that we face if we make it a generic singleton (and also because the majority of the processing is done in the ExtHandler thread and not in the other threads)
Issue #
PR information
Quality of Code and Contribution Guidelines
This change is