Kotlin / kotlin-jupyter

Kotlin kernel for Jupyter/IPython
Apache License 2.0
1.1k stars 105 forks source link

The kernel hangs if a top-level variable is assigned a recursive data structure #319

Closed strangepleasures closed 2 years ago

strangepleasures commented 2 years ago

Create a recursive data structure and assign it to a variable:

val m = mutableListOf<Any>()
m.add(listOf(m))

The kernel hangs, apparently calculating the variable's toString for the :vars REPL command even though the latter wasn't used. Kernel version: 0.10.0.183, worked in previous versions

ileasile commented 2 years ago

Thanks for reporting, I hope we'll fix it soon. For now, you may use previous version

nikolay-egorov commented 2 years ago

Thanks for the notice, it's indeed from a naive toStringcalculation. Will be fixed

strangepleasures commented 2 years ago

@nikolay-egorov Unfortunately, your fix produces lots of false positives, even val l = listOf(1) is counted as recursive. At the same time, traverseObject doesn't traverse array elements, making it possible to create a truly recursive data structure that wouldn't be recognized as such. Also, in many cases recursive data structures don't result in toString explosion. I don't think that a robust generic solution is possible. Probably the best we could do is to properly handle OOMs and stack overflows.

strangepleasures commented 2 years ago
    override val stringValue: String by lazy {
        try {
            cachedValue.getOrNull().toString()
        } catch (e: VirtualMachineError) {
            when(e) {
                is StackOverflowError -> ${cachedValue.getOrNull()!!::class.simpleName}: recursive structure"
                is OutOfMemoryError -> "${cachedValue.getOrNull()!!::class.simpleName}: too large structure"
                else -> throw e
            }
        }
    }
nikolay-egorov commented 2 years ago

You're right, it's not working properly. My bad. Apparently this bottleneck might happen in many containers with wrappers around data. Since I'm using the same approach in a process of serializing variable state, I guess it absolutely ok to do it here as well. What do you think, @ileasile?

ileasile commented 2 years ago

I think it's ok to support some common use cases separately, but proper error handling is a good point

nikolay-egorov commented 2 years ago

Alright, I'll fix it in PR 314, thank you once again, @strangepleasures!