Repository navigation
[CHANGE] Reuse the same request session on same instance - #110
pablo-estevez-lrn wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Session cleanup needs a deterministic public API, and the reuse behavior lacks regression coverage.
3 open findings
What changed in this PR
Reuses one HTTP session per DataApi instance to enable connection pooling.
Changes:
- Adds session creation and cleanup.
- Routes requests through the shared session.
- Updates the changelog.
| File | Description |
|---|---|
learnosity_sdk/request/dataapi.py |
Introduces and uses a shared request session. |
ChangeLog.md |
Records session reuse. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add docstring to the close method for clarity. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
michael-linnane-lrn
left a comment
There was a problem hiding this comment.
Hey @pablo-estevez-lrn, two comments. We need to be careful when altering existing classes since we don't know how the integrations might be used already. Should be easy to fix with backwards compatibility though
| session = getattr(self, '_session', None) | ||
| if session is not None: | ||
| session.close() |
There was a problem hiding this comment.
❓ Question: Why allow later requests? Would we not just fully drop it and have an actual blank slate?
| session = getattr(self, '_session', None) | |
| if session is not None: | |
| session.close() | |
| if getattr(self, '_session', None) is not None: | |
| self._session.close() | |
| self._session = None |
There was a problem hiding this comment.
2 reasons:
- To avoid an invalid state after close(), when we raise an error
None does not have an attribute post.(after the last commit, will be creating a new session instead) - To use the same known semantic that requests, where close is closing the transport, but not converting to a blank slate.
If you want to destroy the object, you can always usedel()
it will be functionally equivalent, so happy to change if you want
| def __init__(self) -> None: | ||
| self._session = requests.Session() |
There was a problem hiding this comment.
❗️ Pressing: Adding __init__ here makes super().__init__() mandatory for subclasses. DataApi previously inherited object.__init__, and since we could have customers that don't have a version pinned and did the above we would actually break their integration. We might need to make it Optional
There was a problem hiding this comment.
Fair point, if _session is not an attr when we try to do the request, we can create the request. Fixed on f2b5117


Checklist
Feature
Bug
Security
Documentation
ChangeLog.md updated
Tests added
All testsuites passed
make distcompleted successfullyBump package version if release coming