Are Kotlin Sequences assumed to be thread safe? IE...

# gradle

[jlleitschuh]

Are Kotlin Sequences assumed to be thread safe? IE, can two threads both read from a sequence in parallel and not have issues?
If they don't I think I found a bug in the Kotlin compiler's LazyScriptDefinitionProvider.

[octylFractal]

I presume for the most part that they are thread safe if the underlying iterator is

[octylFractal]

but it looks like some transforms may make them non-thread-safe, e.g. TakeSequence https://github.com/JetBrains/kotlin/blob/master/libraries/stdlib/src/kotlin/collections/Sequences.kt#L350

[jlleitschuh]

I'm looking at this failure:
https://github.com/shyiko/ktlint/issues/287

The stack trace lines up with this release and this file.
https://github.com/JetBrains/kotlin/blob/v1.2.51/compiler/psi/src/org/jetbrains/kotlin/script/KotlinScriptDefinitionProvider.kt

[ilya.gorbunov]

Sequence is usually safe in a sense that you can call iterator() function concurrently on it. The returned Iterator object however often contains a mutable state that is not synchronized and therefore is not thread safe.

[jlleitschuh]

Oh awesome! I was hoping someone from JB would jump in!! @ilya.gorbunov

I think the bug is arising because the logic in the compiler assumes that two threads can safely read from the same sequence in parallel.

This logic here:

override fun findScriptDefinition(fileName: String): KotlinScriptDefinition? =
        if (nonScriptFileName(fileName)) null
        else lock.read {
            cachedDefinitions.firstOrNull { it.isScript(fileName) }
        }

I don't think this is accurate.
I think this is why we're getting the rare exception.

I think this is never hit by the normal compiler operation (non-script compilation) because the logic short circuits.

if (nonScriptFileName(fileName)) null

[ilya.gorbunov]

I see that a CachingSequence is used there and it uses synchronization in its iterator. However that iterator can call underlying iterator's hasNext concurrently under read lock and that's not safe. cc @ilya.chernikov

[jlleitschuh]

Oh awesome! I'm so glad we finally have a root cause for this very rare timing bug.

This weirdness has been driving me and my team nuts for months with no good solution. Feels so good to finally have a root cause. Thank you so much for validating this @ilya.gorbunov!!!

[jlleitschuh]

@ilya.gorbunov Would it be best for me to open a PR to try to fix this issue??

[ilya.chernikov]

@jlleitschuh I plan to look into it today or tomorrow at latest, so if you have a fix ready, a PR would be very welcome. But if not - I think it doesn’t make sense to do it in parallel.

[jlleitschuh]

@ilya.chernikov I don't have a PR ready right now.
I was starting to get my environment setup so I could develop code to fix it. But if you've already got it on your TODO list, I can leave it to you.
Since it's got a low user impact, I wasn't sure what kind of priority it was going to have.

If you find yourself unable to dedicate the time to fix it, please let me know and I'll put in the time.
If you do have time to fix it, by all means feel free to take it, you'll probably be able to put in a higher quality fix than I can.

[ilya.chernikov]

Ok, thank you for the reporting and offering help. I think I have to look at it anyway, and if it will be not too difficult to fix, I’ll do it. Otherwise, I’ll come back.

[ilya.chernikov]

@jlleitschuh the fix is included in the first EAP of 1.3.20 (1.3.20-eap-25) which should be already available. Since I do not have any test case at hands, could you, please, test it on your cases and share the results.

[jlleitschuh]

@shyiko Do you have a recommendation for testing this?
Thanks @ilya.chernikov