Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[for discussion] asyncio.client: move cache access out of ClientIO into Protocol #219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

navytux
Copy link
Contributor

@navytux navytux commented Jan 8, 2023

to ensure that we do not store the same information twice into the cache; this may or may not be important


kirr:

for discussion

Extracted from #195

…to ensure that we do not store the same information twice into the cache; this may or may not be important)

--------
kirr:

	for discussion

Extracted from #195
@navytux
Copy link
Contributor Author

navytux commented Jan 8, 2023

See "a) moving cache access out of ClientIO into Protocol ..." in #195 (comment) .

@d-maurer
Copy link
Contributor

#195 does not try to move the cache access out of ClientIO to the protocol but have it in the protocol as well.

The cache access in ClientIO is there to be able to deliver cached data even when the server connection is down (i.e. we do not have a protocol instance). In real life, this is likely not necessary because ClientStorage has already tried a cache access and will only use lower level calls when this has failed. However, there is a test which verifies the behavior.

The cache access in ClientIO usually makes the one in Protocol.load_before redundant, unless the client has not been "operational" (i.e. did not have an associated protocol). In the latter case, the ClientIO cache access and the one in load_before do not happen in the same coroutine step and therefore, the cache might have been modified in the meantime.

I do not know what happens when information is stored in the cache which is already there. The original code tried hard to avoid this - maybe, it is important.

Base automatically changed from knext to master January 18, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants