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

Memory leak in coroutines 1.4.x #2564

Closed
sunpeaksfivemile opened this issue Mar 4, 2021 · 4 comments
Closed

Memory leak in coroutines 1.4.x #2564

sunpeaksfivemile opened this issue Mar 4, 2021 · 4 comments
Assignees
Labels

Comments

@sunpeaksfivemile
Copy link

sunpeaksfivemile commented Mar 4, 2021

I've encountered a memory leak which I believe is caused by a race condition in the coroutine library.

I've simplified the leak down to the following repro code:

private suspend fun <T : Any> ReceiveChannel<T>.receiveBatch(size: Int): List<T> {
    return List(size) { receive() }
}

fun main() = runBlocking {
    class Thing(val counter: Long /* for debugging on the heap dump */)

    val channel1 = produce(capacity = 64) { // from the main thread
        (0 until Long.MAX_VALUE).forEach {
            send(Thing(it))
        }
    }

    launch(newSingleThreadContext("Channel2")) { // to a single thread (This thread is important! no leak if everything is single threaded)
        flow {
            while (true) {
                emit(channel1.receiveBatch(64))
            }
        }.collect { }
    }

    Unit
}

This sample code leaks Things very rapidly

Yourkit screenshot

I haven't dug too deep, but one interesting thing is if I inline the code for receive batch, there is no leak. Example:

fun main() = runBlocking {
    class Thing(val counter: Long /* for debugging on the heap dump */)

    val channel1 = produce(capacity = 64) { // from the main thread
        (0 until Long.MAX_VALUE).forEach {
            send(Thing(it))
        }
    }

    launch(newSingleThreadContext("Channel2")) { // to a single thread (This thread is important! no leak if everything is single threaded)
        flow {
            while (true) {
                emit(List(64) { channel1.receive() })
            }
        }.collect { }
    }

    Unit
}

Tested with kotlin 1.4.30 and coroutines-core 1.4.3

@IRus
Copy link
Contributor

IRus commented Mar 9, 2021

Able to reproduce

@qwwdfsad qwwdfsad added the bug label Mar 9, 2021
@qwwdfsad
Copy link
Member

qwwdfsad commented Mar 9, 2021

Great job with a simple reproducer! Investigating this

@qwwdfsad qwwdfsad self-assigned this Mar 9, 2021
qwwdfsad added a commit that referenced this issue Mar 13, 2021
    * Update initCancellability documentation and implementation to be aligned with current invariants
    * Make parentHandle non-volatile and ensure there are no races around it
    * Establish new reusability invariants
      - Reusable continuation can be used _only_ if it's state is not REUSABLE_CLAIMED
      - If it is, spin-loop and wait for release
      - Now the parent is attached to reusable continuation only if it was suspended at least once. Otherwise, the state machine can return via fast-path and noone will be able to release intercepted continuation (-> detach from parent)
      - It implies that the parent is attached after trySuspend call and can be concurrently reused, this is where new invariant comes into play

Fixes #2564
@shmuelr
Copy link
Contributor

shmuelr commented Apr 9, 2021

Hey, it seems this is marked as fixed in #2581 along with a corresponding test https://github.com/Kotlin/kotlinx.coroutines/pull/2581/files#diff-ed434c42575944931c44e85218440a92177bd346ad6791cf22c45820c75413e7

Is this still an issue?
Thanks!

@qwwdfsad
Copy link
Member

It will be fixed in an upcoming release. Version 1.4.3 still has this issue

pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this issue Sep 14, 2022
* Rework reusability control in cancellable continuation

    * Update initCancellability documentation and implementation to be aligned with current invariants
    * Make parentHandle non-volatile and ensure there are no races around it
    * Establish new reusability invariants
      - Reusable continuation can be used _only_ if it states is not REUSABLE_CLAIMED
      - If it is, spin-loop and wait for release
      - Now the parent is attached to reusable continuation only if it was suspended at least once. Otherwise, the state machine can return via fast-path and no one will be able to release intercepted continuation (-> detach from parent)
      - It implies that the parent is attached after trySuspend call and can be concurrently reused, this is where new invariant comes into play
    * Leverage the fact that it's non-atomic and do not check it for cancellation prematurely. It increases the performance of fast-path, but potentially affects rare cancellation cases

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

No branches or pull requests

4 participants