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.

Sign up for GitHub

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

Jump to bottom

[BUG]: InMemoryBlockingCache is not thread-safe #5354

Open
masclot opened this issue Mar 7, 2024 · 2 comments · May be fixed by #5608
Open

[BUG]: InMemoryBlockingCache is not thread-safe #5354

masclot opened this issue Mar 7, 2024 · 2 comments · May be fixed by #5608
Assignees
@manas-yu
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Milestone
1.x GA Follow-up ...

Comments

@masclot
Copy link
Collaborator

masclot commented Mar 7, 2024 •
edited
Loading

Describe the bug

InMemoryBlockingCache needs to process CRUD operations sequentially. To do so, it launches coroutines in a scope with a single-threaded dispatcher. However, using a single-thread does not guarantee sequentiality nor atomicity when combined with coroutines:

  • there is no guarantee of execution order when several launched coroutines are waiting
  • every time a suspend method is called, there is a chance that another coroutine will resume execution

Potential solutions include:

Steps To Reproduce

No steps, errors can happen randomly. Might be related to #5064.

Expected Behavior

The class should be thread-safe

Screenshots/Videos

No response

What device/emulator are you using?

No response

Which Android version is your device/emulator running?

No response

Which version of the Oppia Android app are you using?

No response

Additional Context

No response
@masclot masclot added bug End user-perceivable behaviors which are not desirable. triage needed labels Mar 7, 2024
@masclot
Copy link
Collaborator Author

masclot commented Mar 7, 2024 •
edited
Loading

Sample test within InMemoryBlockingCacheTest that would fail with the current implementation:

  @Test
  fun testSequentiality() = runBlocking {
    val first = Job()
    val second = Job()

    val cache = cacheFactory.create<String>()
    val firstUpdate = cache.updateAsync {
      first.join()
      "first"
    }
    val secondUpdate = cache.updateAsync {
      second.join()
      "second"
    }
    second.complete()
    testCoroutineDispatchers.advanceUntilIdle()
    first.complete()
    testCoroutineDispatchers.advanceUntilIdle()
    firstUpdate.await()
    secondUpdate.await()
    assertThat(awaitCompletion(cache.readAsync())).isEqualTo("second")
  }

@BenHenning
Copy link
Member

Ah I didn't realize that Job could be used as a stand-in replacement for a completable future--that's actually really useful to know.

It's great you devised a test that should fail. It looks like it should be consistent, but I can't be sure without running it. That should be a great test to use for high confidence verification of the final implementation.

@BenHenning BenHenning added this to the 1.0 Global availability milestone Mar 11, 2024
@adhiamboperes adhiamboperes added Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. and removed triage needed labels Mar 12, 2024
@manas-yu manas-yu self-assigned this Dec 19, 2024
@manas-yu manas-yu linked a pull request Dec 19, 2024 that will close this issue
Draft
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@manas-yu manas-yu

Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Projects
[Team] Developer Workflow & Infrastructure - Android
Status: Todo
Milestone
1.x GA Follow-up Items
Development

Successfully merging a pull request may close this issue.

Fix #5354: Replaced single-threaded scope with Actor manas-yu/oppia-android
4 participants
@BenHenning @adhiamboperes @masclot @manas-yu