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

Prevent IllegalArgumentException #421

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Conversation

jbonzohln
Copy link
Contributor

When running my tests I get this error:

java.lang.IllegalArgumentException: Count 'n' must be non-negative, but was -5.

	at kotlin.text.StringsKt__StringsJVMKt.repeat(StringsJVM.kt:775)
	at org.jetbrains.kotlinx.lincheck.strategy.managed.TraceNodePrefixFactory.prefixForNode$lambda$5(TraceReporter.kt:628)
	at org.jetbrains.kotlinx.lincheck.strategy.managed.TraceNode.getPrefix(TraceReporter.kt:354)
	at org.jetbrains.kotlinx.lincheck.strategy.managed.TraceLeafEvent.addRepresentationTo(TraceReporter.kt:415)
	at org.jetbrains.kotlinx.lincheck.strategy.managed.TraceReporterKt.traceGraphToRepresentationList(TraceReporter.kt:343)
	at org.jetbrains.kotlinx.lincheck.strategy.managed.TraceReporterKt.appendShortTrace(TraceReporter.kt:43)
	at org.jetbrains.kotlinx.lincheck.strategy.managed.TraceReporterKt.appendTrace(TraceReporter.kt:31)
	at org.jetbrains.kotlinx.lincheck.ReporterKt.appendFailure(Reporter.kt:392)
	at org.jetbrains.kotlinx.lincheck.strategy.LincheckFailure.toString(LincheckFailure.kt:22)
	at java.base/java.lang.StringConcatHelper.stringOf(StringConcatHelper.java:467)
	at java.base/java.lang.StringConcatHelper.simpleConcat(StringConcatHelper.java:422)
	at org.jetbrains.kotlinx.lincheck.LincheckAssertionError.<init>(LincheckAssertionError.kt:17)
	at org.jetbrains.kotlinx.lincheck.LinChecker$check$1.invoke(LinChecker.kt:48)
	at org.jetbrains.kotlinx.lincheck.LinChecker$check$1.invoke(LinChecker.kt:47)
	at org.jetbrains.kotlinx.lincheck.LinChecker.checkImpl$lincheck(LinChecker.kt:67)
	at org.jetbrains.kotlinx.lincheck.LinChecker.check(LinChecker.kt:47)
	at org.jetbrains.kotlinx.lincheck.LinChecker$Companion.check(LinChecker.kt:195)
	at org.jetbrains.kotlinx.lincheck.LinChecker.check(LinChecker.kt)

@eupp eupp requested a review from avpotapov00 October 29, 2024 12:33
@eupp
Copy link
Collaborator

eupp commented Oct 29, 2024

Could be related to #404

@avpotapov00
Copy link
Collaborator

Hi, @jbonzohln
Thanks for the PR!
Could you please provide the test that caused the failure?
Let's fix it a little bit in a different way.

@jbonzohln
Copy link
Contributor Author

@avpotapov00 No, sorry it's company code I can't share. I believe the cause is a very deep call stack though.

@ndkoval
Copy link
Collaborator

ndkoval commented Nov 27, 2024

Unfortunately, we cannot accept such a PR without tests.

@ndkoval ndkoval closed this Nov 27, 2024
@eupp
Copy link
Collaborator

eupp commented Jan 24, 2025

Hi @ndkoval !
Can we please re-open and merge this PR?

I've tested it on #404 (same issue). I've tried to apply this commit on top of master (as of Lincheck-2.34), and with this commit the problem is no longer reproducible.

Also, I've encountered the same bug while working on another feature --- supporting coroutines in GPMC API #411, and with this fix the problem is also fixed there.

The fix in this PR is very simple, it just prevents the negative value to leak into a function not expecting negative value, by simply taking max(0, x).

As for the test, the problem is that the code affected by this fix is deep down the internal logic of constructing trace representation. It is hard to construct an artificial test to trigger this behavior (I've tried).

We can try to port the reproducing test reported by @de-shyt in #404 from her repository. However, this test is quite large and complicated (it includes custom implementation of Channel interface). Besides, this test is also affected by another issue #403, and when #403 is fixed the issue #404 no longer reproduces.
So my point is that trying to adapt this complicated test for this simple one-liner fix probably does not worth the hustle.

@eupp eupp reopened this Jan 24, 2025
@eupp
Copy link
Collaborator

eupp commented Jan 24, 2025

I also believe max in this line was simply forgotten initially, because other branches of this function use max to prevent negative values.

@ndkoval ndkoval merged commit 5bb2c73 into JetBrains:master Jan 24, 2025
ndkoval added a commit that referenced this pull request Jan 24, 2025
@ndkoval
Copy link
Collaborator

ndkoval commented Jan 24, 2025

@eupp I've merged the PR but accidentally did that to master. Please port the fix to develop as well.

eupp pushed a commit that referenced this pull request Jan 24, 2025
@jbonzohln jbonzohln deleted the patch-1 branch January 24, 2025 16:40
ndkoval pushed a commit that referenced this pull request Jan 27, 2025
Signed-off-by: Jesse Bonzo <[email protected]>
(cherry picked from commit 5bb2c73)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants