Regroup all session creation in util get_localAPI_session or APISession class. - #129
Regroup all session creation in util get_localAPI_session or APISession class.#129Ythogtha wants to merge 9 commits into
Conversation
0f82c30 to
be646e3
Compare
e1507b2 to
c0658d5
Compare
eae5e31 to
cee2b15
Compare
bc30ef6 to
1dfb701
Compare
cee2b15 to
4a333e5
Compare
Millefeuille42
left a comment
Millefeuille42
left a comment
There was a problem hiding this comment.
All good for me except the remaining import util
bd3737d to
119cae3
Compare
e712aa5 to
fce2d13
Compare
8173b33 to
3c34049
Compare
3c34049 to
95a0211
Compare
fb254f1 to
c4d891b
Compare
Millefeuille42
left a comment
Millefeuille42
left a comment
There was a problem hiding this comment.
OK for me if and when all pending comments are resolved.
| class ApiSession(contextlib.AbstractContextManager): | ||
| session=None | ||
|
|
There was a problem hiding this comment.
Thinking about it, could it be interesting to define/override __call__ to login and return session? This would avoid the long and redundant api_session.session and reduce it to api_session().
Not sure if it's very clear and explicit when reading the code thought.
This is a non blocking comment, only putting it here to start a discussion.
There was a problem hiding this comment.
I guess we can have a card about that.
There was a problem hiding this comment.
The problem being that we need to keep a reference to the ApiSession object. If we keep only the session object, the ApiSession instance is destroyed, and the session closed.
And that is the whole point of this PR: make sure sessions are closed.
So we'll keep having an ApiSession object, and use api_session.session...
I'm open to other ideas, but here I tried not to alter, or wrap, the session object, and compose with all the preexisting different methods used for opening and not-always-closing sessions.
Hence, for now, I'd prefer to keep it that way.
It'll be easier later to harmonize ApiSession usages, and simplify it.
There was a problem hiding this comment.
My comment wasn't about removing the ApiSession object, but rather overriding the __call__ method. Which won't affect the lifetime of the object and rather simplify it's usage.
There was a problem hiding this comment.
Thinking about it, overriding __call__ can be hard to read and can introduce issues when auto mocking.
What we can do instead is to rename the class to something that doesn't end in session like XapiSessionManager. Since the object itself doesn't represent the session but a manager over the said object.
| class ApiSession(contextlib.AbstractContextManager): | ||
| session=None | ||
|
|
There was a problem hiding this comment.
I guess we can have a card about that.
1825cbe to
9978d7f
Compare
b4dd99a to
30447d3
Compare
|
|
||
| @override | ||
| def __exit__(self, _type, _value, _traceback): | ||
| self.logout(log=f"logout exception[{_type}] {_value}" if _type else "logout context") |
There was a problem hiding this comment.
Maybe refine the log messages to make complete sentences like logout on context exit and logout on exception
Millefeuille42
left a comment
Millefeuille42
left a comment
There was a problem hiding this comment.
OK for me. Not a fan of having a lot of api_session.session, hence the suggestion maybe to rename the class. Ok for me to skip this / create a card for it if that's not's a blocker for the other reviewers.
85d4bc6 to
285450d
Compare
Wescoeur
left a comment
Wescoeur
left a comment
There was a problem hiding this comment.
Code must be rebased before merge, otherwise nothing to add, perfect!
|
I will create a kanban card regarding the point raised by @Millefeuille42. |
…on class. New APISession class to handle lifecycle of the session, with context, or atexit handlers. Use various originator for xapi session creation, to differentiate where they are being created. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
Do not SMlog information when __del__ is called and the session has already been closed. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
… are set. SMlogs are set to LOG_DEBUG, except on failure to create a session. Fixed some less readable code: - lcache.py: import util on top instead of inside a method - util.py; removed double tempfile import Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
…out calls, privatise some APISession methods. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
Also fix some strings. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
…in cleanup.py Due to the rebase, some old-style session management appeared, this commit makes them use the new APISession. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
Reworked some APISession originator for better consistency. Removed unuseful `session` intermediate variables. Reworked restevdis to avoid a huge indentation caused by APISession, by using a function. Adds a try/finally for session.logout() in util.py#sr_get_capability(). Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>
30447d3 to
b813746
Compare
3 participants
New
APISessionclass to handle lifecycle of the session, with context, oratexithandlers.Use various originator for
xapisession creation, to differentiate where they are being created.APISessionusesget_localAPI_session()for session creation, so there is only one place for session creation, usingxenapi.login_with_password.get_localAPI_sessionshould be avoided and only remain inlcache.py.The session
originatoris shown on all logs messages pertaining to sessions, in SMlog, in the formatAPISession [{originator}] {message}.All login and logout messages starts with login or logout hence it is possible to count them to check if there is session leakage :