[monitorlib] Make UTMClientSession a singleton#1409
[monitorlib] Make UTMClientSession a singleton#1409the-glu wants to merge 1 commit intointeruss:mainfrom
Conversation
|
I do think we want something functionally like this (there is no reason to have two separate instances with identical construction parameters), but
I don't think that's correct if "each test" is referring to test scenarios. Nearly all test scenarios obtain their UTMClientSession from a resource's single The only slightly-questionable design I see is uss_qualifier.scenarios.astm.utm.make_uss_report where a separate UTMClientSession is created at test scenario run time to avoid the need to create an "F3548 USS" resource type and declaration just for this one endpoint when that information can be extracted from previous test operations. But, this test scenario is only run once near the very end of the only test suite in which it's used, so I don't see how it could contribute appreciably to the problem. Therefore, I'm surprised that this PR has any appreciable effect. Could you elaborate on your observations re: "Tested manually by checking the number of open sockets that stay to a reasonable value."? If we added a log statement to the constructor of UTMClientSession (post singleton-pattern check in this PR), I would expect to see roughly the same number of log statements after this PR as before. Or, via alternate observation, I would expect this hypothetical log statement to be triggered very rarely (only when the base URL for two different APIs happened to be the same): if hasattr(
self, "_prefix_url"
): # If set, we have been reused from the singleton pattern, no need to do anything
logger.info("Reusing existing UTMClientSession with identical parameters")
returnDo you expect to see something different than those two observations? If not, how would this PR be expected to improve #1407? Separately, I would rather implement this functionality via a factory pattern to satisfy the principle of least surprise. The implementation in this PR certainly has the fewest lines changed, but it seems like it sets a behavior-differs-from-expectations landmine for the future. Instead, it seems like we can achieve the same result without the landmine by creating a UTMClientSessionFactory + singleton and then replacing all existing instantiations of UTMClientSession with a call to the UTMClientSessionFactory singleton's |
With some debug statements, running As you can see there are indeed a lot of duplicate Adding logging on scenario creation, we can see that the ones generated by Even on a single scenario, resources are not reused (uss1 instanced twice): Is there an issue somewhere else? Because if you expect resources to be reused in general, they may be something wrong elsewhere and we shouldn't need to make it a singleton. |
|
We did some debugging with @mickmis and the root cause seems to be that: Test scenarios use get_instance create a new DSSInstance every time: https://github.com/interuss/monitoring/blob/main/monitoring/uss_qualifier/resources/astm/f3548/v21/dss.py#L771And DSSInstance create a new UTMClientSession every time: https://github.com/interuss/monitoring/blob/main/monitoring/uss_qualifier/resources/astm/f3548/v21/dss.py#L94
Should |
Ah, there we go -- DSSInstance and DSSInstanceResource are treating UTMClientSessions as cheap. That's a great discovery and something I think we can fix closer to the root.
Each |
|
Yes :) |
Proposition 2 to fix connections leaking. See #1408 for alternative version (or we could have both, but that probably not needed).
The test suite is currently creating a set of
UTMClientSessionduring initialization, for each test and endpoint used.This create issues, as connections use keep-alive mechanism and are never closed. #1407 is an example of that with a lot USS, reaching the limit after a while.
This version of the fix make
UTMClientSessiona singleton, based on it parameters, meaning it will be reused across tests.This may have side effects, but I'm not sure how it's could be used across the whole codebase (eg. if the session / AuthAdapter are dynamically modified).
Tested manually by checking the number of open sockets that stay to a reasonable value. Connections are still reused.