Feature/refactor lincheck#255
Conversation
…earchManager.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughAdds coroutine-based cold flows for search and stock prices, new LinCheck-based concurrency tests (and removals of older linearizability tests), configuration and metadata updates (Kotlin bump, lincheck bump, editorconfig import rules), small code fixes, and gitignore/local.properties adjustments. Changes
Sequence DiagramssequenceDiagram
actor Client
participant SearchManager
participant QueryFlow as Flow<String>
participant SearchApi
participant ResultsFlow as Flow<List<String>>
Client->>SearchManager: getSuggestions(queryFlow, debounceMillis)
SearchManager->>QueryFlow: debounce → filter → distinctUntilChanged
loop for each unique debounced query
SearchManager->>SearchApi: search(query)
SearchApi-->>SearchManager: List<String>
SearchManager->>ResultsFlow: emit(results)
end
ResultsFlow-->>Client: emit List<String>
sequenceDiagram
actor Client
participant StockRepository
participant Ticker as tickerFlow(interval)
participant StockApi
participant PriceFlow as Flow<Double>
Client->>StockRepository: getStockPrice(symbol)
StockRepository->>Ticker: create ticker at refreshInterval
loop continuous ticks
Ticker->>StockRepository: emit Unit
StockRepository->>StockApi: fetchPrice(symbol)
StockApi-->>StockRepository: Double
StockRepository->>PriceFlow: emit price
end
PriceFlow-->>Client: emit Double
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@api/Kotlin-Lab.api`:
- Around line 19791-19794: Move the test/demo implementation class FakeSearchApi
(defined in SearchApi.kt under package
dev.shtanko.concurrency.coroutines.flow.cold) out of the main source set and
into the test source set: create
src/test/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/FakeSearchApi.kt
with the same package and class definition, remove the FakeSearchApi from
src/main to avoid exposing the test double in the public API, and ensure the
test suite compiles (no import changes should be necessary because tests share
the same package).
In `@src/main/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/StockApi.kt`:
- Around line 19-25: getStockPrice in StockRepository uses
tickerFlow(refreshInterval).map { api.fetchPrice(symbol) } with no error
handling, so any exception from api.fetchPrice will terminate the polling Flow;
modify getStockPrice to handle failures by adding a retry/backoff or a catch
that recovers (e.g., use .retryWhen or .retry with exponential backoff around
the mapping, or append .catch after map to emit a fallback value or continue) so
tickerFlow keeps emitting after transient api.fetchPrice errors; locate
StockRepository.getStockPrice, tickerFlow usage, and api.fetchPrice to implement
the chosen retry/catch strategy.
In `@src/main/kotlin/dev/shtanko/datastructures/stacks/IntStack.kt`:
- Around line 112-115: The indexOf method iterates one past the logical top
because topOfStack is the next empty slot (see push behaviour), so change the
loop to only scan valid elements: replace the inclusive range using topOfStack
with a half-open range up to topOfStack (e.g., use 0 until topOfStack) when
iterating over elements in indexOf to avoid reading elements[topOfStack] and
potential OOB or false matches.
In `@src/test/kotlin/dev/shtanko/collections/concurrent/ConcurrentHashMapTest.kt`:
- Around line 10-42: Remove the redundant ConcurrentHashMapTest class (the class
named ConcurrentHashMapTest and its methods
put/get/remove/stressTest/modelCheckingTest) because JavaConcurrentHashMapTest
already covers the same operations via AbstractConcurrentMapTest; delete this
test class or replace it with a variant that provides distinct scenarios or
parameters not already exercised by JavaConcurrentHashMapTest (e.g., different
thread/iteration/invocation counts or additional operation sequences) to avoid
duplicate CI work.
In `@src/test/kotlin/dev/shtanko/collections/concurrent/CounterTest.kt`:
- Around line 27-34: The inline comments for iterations and
invocationsPerIteration in the stressTest() builder are incorrect (they claim
100 and 1000 but the values are 50 and 500); update the comments to reflect the
actual values or remove them. Locate the stressTest() function that builds a
StressOptions() chain and either change the comment on .iterations(50) to state
"Generate 50 random concurrent scenarios" and the comment on
.invocationsPerIteration(500) to "Run each generated scenario 500 times", or
delete both misleading comments so the code values are the single source of
truth.
- Around line 9-10: The `@DisabledIfEnvironmentVariable` annotation is applied to
the private helper class Counter so it has no effect; move the annotation to the
test class CounterTest so JUnit will disable the tests in CI. Remove or relocate
`@DisabledIfEnvironmentVariable` from the Counter class declaration and add it
directly above the CounterTest class declaration (keeping the same named="CI"
matches="true" parameters) so the test class honors the environment-based
disablement.
- Around line 4-5: The import for StressOptions is wrong — replace the import
that currently references org.jetbrains.lincheck.datastructures with the root
package import for StressOptions (keep the existing Operation import as-is);
locate the import of StressOptions in CounterTest.kt (symbol: StressOptions) and
change it to use org.jetbrains.lincheck so the test uses the correct LinCheck
class.
🧹 Nitpick comments (5)
.github/workflows/build.yml (1)
68-72: Consider removing the dead step entirely rather than commenting it out.Commented-out CI steps tend to linger and become stale. If the Codacy integration is no longer needed, removing the block keeps the workflow clean. If it might be re-enabled later, note that the coverage path (
jacocoTestReport.xml) doesn't match the Kover report generated on line 66 — it would need updating regardless.src/main/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/SearchManager.kt (1)
13-26: Clean flow composition — consider logging errors in the catch block.The pipeline logic is well-structured and idiomatic. One minor concern: the
.catch { emit(emptyList()) }on line 24 silently swallows all exceptions. For production debugging, consider logging the throwable (e.g., via a logger or an injected error handler) before emitting the fallback.src/main/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/StockApi.kt (1)
12-17:tickerFlowdoes not validate theintervalparameter.A zero or negative
intervalwould causedelayto either spin without yielding (0) or throw anIllegalArgumentException(negative). Consider adding arequire(interval > 0)guard.Proposed fix
fun tickerFlow(interval: Long) = flow { + require(interval > 0) { "Interval must be positive, was $interval" } while (true) { emit(Unit) delay(interval) } }src/test/kotlin/dev/shtanko/collections/concurrent/AbstractConcurrentMapTest.kt (1)
11-11:outvariance onTis unnecessary and misleading.
Tis only used in a private constructor parameter, soouthas no practical effect. Since the map is mutated viaput/remove, dropping theoutmodifier better conveys the intent.Suggested change
-abstract class AbstractConcurrentMapTest<out T : MutableMap<Int, Int>>(private val map: T) { +abstract class AbstractConcurrentMapTest<T : MutableMap<Int, Int>>(private val map: T) {src/test/kotlin/dev/shtanko/collections/concurrent/ConcurrentSkipListMapTest.kt (1)
12-53: Consider extendingAbstractConcurrentMapTestto reduce duplication.
put,get, andremoveare identical to those in the abstract base class. You could extendAbstractConcurrentMapTest<ConcurrentSkipListMap<Int, Int>>and only add thefirstKey/lastKeyoperations plus override the test methods if different thread/iteration counts are needed.
| public final class dev/shtanko/concurrency/coroutines/flow/cold/FakeSearchApi : dev/shtanko/concurrency/coroutines/flow/cold/SearchApi { | ||
| public fun <init> ()V | ||
| public fun search (Ljava/lang/String;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definition of FakeSearchApi
echo "=== FakeSearchApi definition location ==="
fd "\.kt$" -type f | xargs grep -l "class FakeSearchApi" 2>/dev/null | head -20
echo ""
echo "=== All usages of FakeSearchApi ==="
rg "FakeSearchApi" --type kotlin -n 2>/dev/null | head -50Repository: ashtanko/Kotlin-Lab
Length of output: 567
🏁 Script executed:
cat -n src/main/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/SearchApi.ktRepository: ashtanko/Kotlin-Lab
Length of output: 723
🏁 Script executed:
cat -n src/test/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/SearchManagerTest.kt | head -30Repository: ashtanko/Kotlin-Lab
Length of output: 1321
Move FakeSearchApi to the test source set.
FakeSearchApi is defined in src/main/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/SearchApi.kt but only used in tests. The source comment explicitly marks it as a test/demo implementation. Test doubles should reside in src/test to avoid polluting the public API surface. Move the class to src/test/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/ — since the test file is in the same package, no import changes are needed.
🤖 Prompt for AI Agents
In `@api/Kotlin-Lab.api` around lines 19791 - 19794, Move the test/demo
implementation class FakeSearchApi (defined in SearchApi.kt under package
dev.shtanko.concurrency.coroutines.flow.cold) out of the main source set and
into the test source set: create
src/test/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/FakeSearchApi.kt
with the same package and class definition, remove the FakeSearchApi from
src/main to avoid exposing the test double in the public API, and ensure the
test suite compiles (no import changes should be necessary because tests share
the same package).
| class StockRepository( | ||
| private val api: StockApi, | ||
| private val refreshInterval: Long = 5000L, | ||
| ) { | ||
| fun getStockPrice(symbol: String): Flow<Double> = | ||
| tickerFlow(refreshInterval).map { api.fetchPrice(symbol) } | ||
| } |
There was a problem hiding this comment.
No error handling — a single fetchPrice failure will terminate the flow.
Unlike SearchManager.getSuggestions, which catches API errors and emits a fallback, StockRepository.getStockPrice has no error handling. If api.fetchPrice throws, the entire polling flow terminates permanently. For a long-lived polling flow, consider adding .retry or .catch with a recovery strategy.
Example: retry with backoff
+import kotlinx.coroutines.flow.retry
+
class StockRepository(
private val api: StockApi,
private val refreshInterval: Long = 5000L,
) {
fun getStockPrice(symbol: String): Flow<Double> =
- tickerFlow(refreshInterval).map { api.fetchPrice(symbol) }
+ tickerFlow(refreshInterval)
+ .map { api.fetchPrice(symbol) }
+ .retry(3) { cause ->
+ cause is java.io.IOException
+ }
}🤖 Prompt for AI Agents
In `@src/main/kotlin/dev/shtanko/concurrency/coroutines/flow/cold/StockApi.kt`
around lines 19 - 25, getStockPrice in StockRepository uses
tickerFlow(refreshInterval).map { api.fetchPrice(symbol) } with no error
handling, so any exception from api.fetchPrice will terminate the polling Flow;
modify getStockPrice to handle failures by adding a retry/backoff or a catch
that recovers (e.g., use .retryWhen or .retry with exponential backoff around
the mapping, or append .catch after map to emit a fallback value or continue) so
tickerFlow keeps emitting after transient api.fetchPrice errors; locate
StockRepository.getStockPrice, tickerFlow usage, and api.fetchPrice to implement
the chosen retry/catch strategy.
| fun indexOf(value: Int): Int { | ||
| for (i in 0 until topOfStack) if (elements[i] == value) return i | ||
| for (i in 0..topOfStack) if (elements[i] == value) return i | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
Off-by-one bug: 0..topOfStack reads one element past the stack's logical top.
topOfStack points to the next empty slot (post-increment on push at line 41). Valid element indices are 0 until topOfStack. Using 0..topOfStack reads elements[topOfStack], which is either stale/uninitialized data (false-positive match) or throws ArrayIndexOutOfBoundsException when the stack is full.
🐛 Proposed fix
fun indexOf(value: Int): Int {
- for (i in 0..topOfStack) if (elements[i] == value) return i
+ for (i in 0 until topOfStack) if (elements[i] == value) return i
return -1
}🤖 Prompt for AI Agents
In `@src/main/kotlin/dev/shtanko/datastructures/stacks/IntStack.kt` around lines
112 - 115, The indexOf method iterates one past the logical top because
topOfStack is the next empty slot (see push behaviour), so change the loop to
only scan valid elements: replace the inclusive range using topOfStack with a
half-open range up to topOfStack (e.g., use 0 until topOfStack) when iterating
over elements in indexOf to avoid reading elements[topOfStack] and potential OOB
or false matches.
| @DisabledIfEnvironmentVariable(named = "CI", matches = "true") | ||
| class ConcurrentHashMapTest { | ||
|
|
||
| private val map = ConcurrentHashMap<Int, Int>() | ||
|
|
||
| @Operation | ||
| fun put(key: Int, value: Int) = map.put(key, value) | ||
|
|
||
| @Operation | ||
| fun get(key: Int) = map[key] | ||
|
|
||
| @Operation | ||
| fun remove(key: Int) = map.remove(key) | ||
|
|
||
| @Test | ||
| fun stressTest() { | ||
| // Stress testing runs multiple threads to find bugs on the real JVM | ||
| StressOptions() | ||
| .iterations(100) | ||
| .threads(3) | ||
| .invocationsPerIteration(10) | ||
| .check(this::class) | ||
| } | ||
|
|
||
| @Test | ||
| fun modelCheckingTest() { | ||
| // Model checking explores all possible interleavings (more deterministic) | ||
| ModelCheckingOptions() | ||
| .iterations(50) | ||
| .threads(2) | ||
| .invocationsPerIteration(10) | ||
| .check(this::class) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "AbstractConcurrentMapTest.kt" --exec cat {}Repository: ashtanko/Kotlin-Lab
Length of output: 1287
Remove ConcurrentHashMapTest to eliminate test duplication.
JavaConcurrentHashMapTest already inherits the identical stressTest() and modelCheckingTest() methods from AbstractConcurrentMapTest, which tests ConcurrentHashMap<Int, Int> with the same operations and configurations. This new standalone class duplicates that coverage, unnecessarily increases CI time, and creates maintenance burden.
If you need a separate test class for a specific reason, ensure it provides distinct test scenarios or parameters not covered by the abstract base class.
🤖 Prompt for AI Agents
In `@src/test/kotlin/dev/shtanko/collections/concurrent/ConcurrentHashMapTest.kt`
around lines 10 - 42, Remove the redundant ConcurrentHashMapTest class (the
class named ConcurrentHashMapTest and its methods
put/get/remove/stressTest/modelCheckingTest) because JavaConcurrentHashMapTest
already covers the same operations via AbstractConcurrentMapTest; delete this
test class or replace it with a variant that provides distinct scenarios or
parameters not already exercised by JavaConcurrentHashMapTest (e.g., different
thread/iteration/invocation counts or additional operation sequences) to avoid
duplicate CI work.
| import org.jetbrains.lincheck.datastructures.Operation | ||
| import org.jetbrains.lincheck.datastructures.StressOptions |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which lincheck version is declared in the project
rg -n "lincheck" --type=tomlRepository: ashtanko/Kotlin-Lab
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Check if the imports are consistent across all test files in this PR
rg -n "org.jetbrains.lincheck" --type=kotlinRepository: ashtanko/Kotlin-Lab
Length of output: 1995
🌐 Web query:
LinCheck 3.4 package structure org.jetbrains.lincheck.datastructures Operation StressOptions
💡 Result:
Lincheck 3.4 (artifact org.jetbrains.lincheck:lincheck:3.4) is published under the org.jetbrains.lincheck root package. [3]
Operation
- FQN:
org.jetbrains.lincheck.datastructures.Operation - Source location (tag
lincheck-3.4):src/jvm/main/org/jetbrains/lincheck/datastructures/Operation.kt(declarespackage org.jetbrains.lincheck.datastructures) [1]
StressOptions
- FQN:
org.jetbrains.lincheck.StressOptions - Evidence: the official usage examples import
StressOptionsviaimport org.jetbrains.lincheck.*(a star import only brings in types from exactlyorg.jetbrains.lincheck, not subpackages) and then callStressOptions()directly. [2]
Typical imports in 3.4:
import org.jetbrains.lincheck.*
import org.jetbrains.lincheck.datastructures.*[2]
Sources: [1] [2] [3]
Fix incorrect StressOptions import path.
StressOptions is located in org.jetbrains.lincheck, not org.jetbrains.lincheck.datastructures. The Operation import is correct. Update line 5:
import org.jetbrains.lincheck.StressOptions(The org.jetbrains.lincheck.datastructures package is standard for LinCheck 3.4, but StressOptions resides at the root org.jetbrains.lincheck level.)
🤖 Prompt for AI Agents
In `@src/test/kotlin/dev/shtanko/collections/concurrent/CounterTest.kt` around
lines 4 - 5, The import for StressOptions is wrong — replace the import that
currently references org.jetbrains.lincheck.datastructures with the root package
import for StressOptions (keep the existing Operation import as-is); locate the
import of StressOptions in CounterTest.kt (symbol: StressOptions) and change it
to use org.jetbrains.lincheck so the test uses the correct LinCheck class.
| fun stressTest() = StressOptions() // Stress testing options: | ||
| .actorsBefore(2) // Number of operations before the parallel part | ||
| .threads(2) // Number of threads in the parallel part | ||
| .actorsPerThread(2) // Number of operations in each thread of the parallel part | ||
| .actorsAfter(1) // Number of operations after the parallel part | ||
| .iterations(50) // Generate 100 random concurrent scenarios | ||
| .invocationsPerIteration(500) // Run each generated scenario 1000 times | ||
| .check(this::class) // Run the test |
There was a problem hiding this comment.
Misleading inline comments on iterations and invocationsPerIteration.
Line 32 comment says "Generate 100 random concurrent scenarios" but the value is 50. Line 33 says "Run each generated scenario 1000 times" but the value is 500. Either update the comments to match the actual values or remove them.
Proposed fix
- .iterations(50) // Generate 100 random concurrent scenarios
- .invocationsPerIteration(500) // Run each generated scenario 1000 times
+ .iterations(50) // Generate 50 random concurrent scenarios
+ .invocationsPerIteration(500) // Run each generated scenario 500 times📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun stressTest() = StressOptions() // Stress testing options: | |
| .actorsBefore(2) // Number of operations before the parallel part | |
| .threads(2) // Number of threads in the parallel part | |
| .actorsPerThread(2) // Number of operations in each thread of the parallel part | |
| .actorsAfter(1) // Number of operations after the parallel part | |
| .iterations(50) // Generate 100 random concurrent scenarios | |
| .invocationsPerIteration(500) // Run each generated scenario 1000 times | |
| .check(this::class) // Run the test | |
| fun stressTest() = StressOptions() // Stress testing options: | |
| .actorsBefore(2) // Number of operations before the parallel part | |
| .threads(2) // Number of threads in the parallel part | |
| .actorsPerThread(2) // Number of operations in each thread of the parallel part | |
| .actorsAfter(1) // Number of operations after the parallel part | |
| .iterations(50) // Generate 50 random concurrent scenarios | |
| .invocationsPerIteration(500) // Run each generated scenario 500 times | |
| .check(this::class) // Run the test |
🤖 Prompt for AI Agents
In `@src/test/kotlin/dev/shtanko/collections/concurrent/CounterTest.kt` around
lines 27 - 34, The inline comments for iterations and invocationsPerIteration in
the stressTest() builder are incorrect (they claim 100 and 1000 but the values
are 50 and 500); update the comments to reflect the actual values or remove
them. Locate the stressTest() function that builds a StressOptions() chain and
either change the comment on .iterations(50) to state "Generate 50 random
concurrent scenarios" and the comment on .invocationsPerIteration(500) to "Run
each generated scenario 500 times", or delete both misleading comments so the
code values are the single source of truth.
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests