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

Parent coroutine is frozen when child coroutine on background thread is launched #1745

Closed
brendanw opened this issue Jan 3, 2020 · 11 comments

Comments

@brendanw
Copy link

brendanw commented Jan 3, 2020

coroutines_version=1.3.3-native-mt
kotlin_version=1.3.61

GlobalScope.launch(Dispatchers.Main) {
  this.ensureNeverFrozen()
  val result = someBackgroundSuspend()
  // `this` is now frozen. any other object referenced in this lambda is now frozen as well
}

suspend fun someBackgroundSuspend() = withContext(Dispatchers.Default) {
  true
}

expected result: parent coroutine should not be frozen on native.
actual result: parent coroutine is frozen on native; makes it impossible to dispatch result to any listeners without using atomics everywhere to prevent mutability exceptions.

When I am running something akin to this in my project I am actually seeing someBackgroundSuspend() never complete when I have the ensureNeverFrozen call beforehand.

GlobalScope.launch(Dispatchers.Main) {
  this.ensureNeverFrozen()
  val scope = CoroutineScope(Dispatchers.Default)
  val job = scope.launch { 
                
  }
  // `this` is not frozen
}
@brendanw
Copy link
Author

brendanw commented Jan 3, 2020

Adding the below test to MainDispatcherTest under nativeDarwin shows what is problematic:

    @Test
    fun testParentIsFrozen() {
        runTest {
            withContext(Dispatchers.Default) {
                "anything"
            }
            assertEquals(this.isFrozen, false)
        }
    }

@elizarov
Copy link
Contributor

elizarov commented Jan 15, 2020

Can you, please, give more details on how freezing this freezing affects your code. The test you've posted, as far as I can see, behaves correctly. You see, this inside runTest { ... } refers to the coroutine's internal Job object which becomes shared across threads and thus must be frozen. It does not refer to the lambda object itself.

You have the following comment in your issue:

this is now frozen. any other object referenced in this lambda is now frozen as well

However, I cannot reproduce the second part of your problem that "any other object referenced in this lambda is now frozen as well". To be sure, I've created the following test and it runs just fine:

    @Test
    fun testParentIsFrozen() = runTest {
        // create a mutable object referenced by this lambda
        val mutable = mutableListOf<Int>()
        // run a child coroutine in another thread
        val result = withContext(Dispatchers.Default) { "OK" }
        // ensure that objects referenced by this lambda were not frozen
        assertFalse(mutable.isFrozen)
        mutable.add(42) // just to be 100% sure
    }

@brendanw
Copy link
Author

The test you wrote above works. If I change it slightly, then the test hangs:

  @Test
  fun sameThreadFreeze() {
    runTest {
      val channel = Channel<Int>(Channel.UNLIMITED)
      launch {
        assertEquals(channel.isFrozen, false)
        val str = withContext(Dispatchers.Main) {
          "anything"
        }
        assertEquals(channel.isFrozen, false)
      }
    }
  }

image

If I run the above on the simulator and replace assertEquals with println(channel.isFrozen) I see false printed before withContext and true printed after.

@elizarov
Copy link
Contributor

@brendanw This test hangs because you cannot do withContext(Dispatchers.Main) from here. However, if you change to withContext(Dispatchers.Default) then it works fine for me (channel is not frozen).

@brendanw
Copy link
Author

I will work on trying to reproduce the case I am seeing in my app in a test; if I cannot reproduce in a test I will close this issue.

For reference, I am using https://github.com/freeletics/CoRedux to build a class RankingsStateMachine. The class is pure kotlin code to be shared by both an iOS app and android app. I replaced a java ReentrantLock in CoRedux with atomicfu's ReentrantLock. To be sincere, I don't fully understand the code in CoRedux; if I did, it would probably be easier to make an isolated test case to show the problem.

@brendanw
Copy link
Author

brendanw commented Jan 16, 2020

    @Test
    fun testParentIsFrozen() = runTest {
        // create a mutable object referenced by this lambda
        val mutable = mutableListOf<Int>()
        // run a child coroutine in another thread
        val result = withContext(Dispatchers.Default) { "OK" }
        // ensure that objects referenced by this lambda were not frozen
        assertFalse(mutable.isFrozen)
        mutable.add(42) // just to be 100% sure
    }

I thought that the trailing lambda parameter passed to runTest would be frozen. The Dispatchers.Default lambda must have some reference to the runTest lambda so it can pass a result back to the runTest lambda.

If this true, then the runTest lambda is being accessed by the thread it is run on and a background thread, so it should be frozen. If the runTest lambda is frozen, then everything the runTest lambda has a reference to should be frozen. And that's why I'd expect mutable to be frozen even though your test shows it is not.

Please help me understand the flaw in my logic.

@elizarov
Copy link
Contributor

Kotlin lambdas don't keep a reference to their enclosing lambdas. They only capture ("close over") objects from the outer lambdas that are explicitly referenced in their code. So, in the testParentIsFrozen if I change the code like this to have a reference to mutable val inside of withContext(Dispatchers.Default) lambda:

val result = withContext(Dispatchers.Default) { 
    println(mutable) // reference mutable val
    "OK" 
}

Then, mutable will get captured into the lambda that is shared with a different thread and will get frozen as a result.

elizarov added a commit that referenced this issue Jan 17, 2020
Trying to figure out #1745
@elizarov
Copy link
Contributor

elizarov commented Jan 17, 2020

@brendanw See this comment: #1648 (comment)
It looks like @Thomas-Vos might have reproduced the issue.
Update: Might be a separate issue: #1764

@brendanw
Copy link
Author

brendanw commented Mar 2, 2020

  @Test
  fun testParentIsFrozen() = runTest {
    // create a mutable object referenced by this lambda
    val mutable = mutableListOf<Int>()
    val mutable2 = mutableListOf<Int>()
    // run a child coroutine in another thread
    val result = withContext(io) {
      mutable2.add(1)
    }
    // ensure that objects referenced by this lambda were not frozen
    assertFalse(mutable.isFrozen)
    mutable.add(42) // just to be 100% sure
  }

@elizarov If I modify the test to the above, the test fails. Is this expected?

@elizarov
Copy link
Contributor

@brendanw This test is expected to fail because mutable2 is transferred from one thread to another, which makes it frozen. You cannot share mutable objects between threads in Kotlin/Native. They have to be frozen to share them.

Does it address your question?

@brendanw
Copy link
Author

Ah, duh. Yup that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants