Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: resolve potential deadlock in AsyncInMemoryCache #4464

Merged

Conversation

dhlidongming
Copy link
Contributor

@dhlidongming dhlidongming commented Nov 8, 2024

This pull request resolves potential concurrency issues in the AsyncInMemoryCache class.

In the upsert function:

async def upsert(self, key, value, lock: asyncio.Lock | None = None) -> None:
if not lock:
async with self.lock:
await self._upsert(key, value)

if the lock parameter is not provided, the self.lock object is used to acquire a lock before calling the _upsert function. Within the _upsert function, the get function is invoked without a lock parameter:
async def _upsert(self, key, value) -> None:
existing_value = await self.get(key)

which causes the get function to acquire the self.lock object again, leading to a deadlock:
async def get(self, key, lock: asyncio.Lock | None = None):
if not lock:
async with self.lock:
return await self._get(key)

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Nov 8, 2024
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Nov 8, 2024
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Nov 8, 2024
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Nov 8, 2024
@dhlidongming dhlidongming changed the title fix: resolve lock misuse and potential deadlock in AsyncInMemoryCache fix: resolve potential deadlock in AsyncInMemoryCache Nov 8, 2024
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Nov 8, 2024
@ogabrielluiz ogabrielluiz requested a review from cbornet November 8, 2024 11:17
@ogabrielluiz ogabrielluiz force-pushed the fix/async-memory-cache-lock-issues branch from 916fae4 to eb733f0 Compare November 8, 2024 11:19
@dhlidongming
Copy link
Contributor Author

dhlidongming commented Nov 8, 2024

@ogabrielluiz The PR contains an error: the lock parameter cannot be removed, even though it is not used.
Because the base class abstract method includes this parameter:

def upsert(self, key, value, lock: LockType | None = None):

It will also be passed in during the call.

@ogabrielluiz
Copy link
Contributor

@ogabrielluiz The PR contains an error: the lock parameter cannot be removed, even though it is not used. Because the base class abstract method includes this parameter:

def upsert(self, key, value, lock: LockType | None = None):

It will also be passed in during the call.

Hey @dhlidongming

Thanks for this fix.

Maybe you could pass the lock all the way down to the get method.

@cbornet
Copy link
Collaborator

cbornet commented Nov 8, 2024

Could also use a rentrant lock asyncio.RLock ?

@@ -355,11 +355,7 @@ async def _clear(self) -> None:
self.cache.clear()

async def upsert(self, key, value, lock: asyncio.Lock | None = None) -> None:
if not lock:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotate the method with @override (from typing_extensions) to solve the ARG002 rule error.

Copy link

codspeed-hq bot commented Nov 9, 2024

CodSpeed Performance Report

Merging #4464 will degrade performances by 15.55%

Comparing dhlidongming:fix/async-memory-cache-lock-issues (8279837) with main (0f5c31f)

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 12 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main dhlidongming:fix/async-memory-cache-lock-issues Change
test_get_all 5,192.8 ms 520.5 ms ×10
test_invalid_run_with_input_type_chat 13.8 ms 16.4 ms -15.55%
test_successful_run_with_input_type_text 172.3 ms 141.6 ms +21.7%

@dhlidongming dhlidongming requested a review from cbornet November 9, 2024 05:51
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Nov 13, 2024
Copy link
Contributor

@ogabrielluiz ogabrielluiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 14, 2024
@ogabrielluiz ogabrielluiz enabled auto-merge (squash) November 14, 2024 11:28
@ogabrielluiz ogabrielluiz merged commit af54655 into langflow-ai:main Nov 14, 2024
19 checks passed
diogocabral pushed a commit to headlinevc/langflow that referenced this pull request Nov 26, 2024
* Fix potential lock misuse and deadlock in AsyncInMemoryCache.

* Recover async lock handling logic.

* Remove unused lock parameter in upsert.

* Fix potential lock misuse and deadlock in AsyncInMemoryCache.

* Recover async lock handling logic.

* Remove unused lock parameter in upsert.

* Add lock parameter to prevent errors.

* Fix ARG002 rule error.

* Lock passed to get and set method.

---------

Co-authored-by: Gabriel Luiz Freitas Almeida <gabriel@langflow.org>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants