Stabilize coroutine execution in ObdDeviceConnection

[eltonvs]

Summary

• remove nested runBlocking from suspend paths in ObdDeviceConnection
• serialize command execution with Mutex to prevent stream interleaving
• make cache deterministic/concurrency-safe via stable cache key
• fix EOF handling by checking InputStream.read() for -1
• keep transport-agnostic design (no Bluetooth/Wi-Fi deps added)

Tests

• add connection-level tests for:
  • serialized concurrent execution
  • concurrent cache behavior
  • EOF/disconnect handling
  • cancellation propagation
• ./gradlew test passes

Docs

• clarify coroutine/background-thread usage for Android
• clarify transport-agnostic support (Bluetooth/Wi-Fi/USB)
• document serialized connection behavior

[greptile-apps]

Greptile Summary

Refactored ObdDeviceConnection to eliminate nested runBlocking in suspend functions and serialize command execution with a Mutex to prevent stream interleaving. Added comprehensive test coverage for concurrent behavior, EOF handling, and cancellation propagation.

Key improvements:

• Removed nested runBlocking from suspend paths (eliminated coroutine anti-pattern)
• Serialized command execution with Mutex to prevent concurrent stream access
• Made cache key deterministic using class name + raw command
• Fixed EOF detection by checking for -1 from InputStream.read()
• Added ioDispatcher parameter for testability
• Comprehensive test suite covering serialization, caching, EOF, and cancellation

Issues found:

• Uninitialized variable risk: rawData in runCommand() could throw UninitializedPropertyAccessException if readRawData() throws before assignment

Confidence Score: 4/5

• This PR is safe to merge after fixing the uninitialized variable issue
• The refactoring correctly addresses coroutine anti-patterns and adds proper serialization. However, one critical issue exists where rawData could remain uninitialized if an exception is thrown. Once that's fixed with a simple initialization, the changes are solid.
• Pay close attention to ObdDeviceConnection.kt line 48 - must initialize rawData to prevent potential runtime exception

Important Files Changed

Filename	Overview
src/main/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnection.kt	Removed nested runBlocking, added Mutex for serialization, improved cache key, fixed EOF handling - one uninitialized variable issue found
src/test/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnectionTest.kt	Added comprehensive tests for serialization, caching, EOF handling, and cancellation - well-structured test coverage
README.md	Updated documentation to clarify coroutine usage, Android threading requirements, and serialized connection behavior

_{Last reviewed commit: 77df86f}

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Summary

Refactored ObdDeviceConnection to fix critical coroutine anti-patterns and race conditions. Removed nested runBlocking calls from suspend functions (which caused thread blocking), added Mutex-based serialization to prevent command interleaving on shared I/O streams, and fixed EOF detection by checking for -1 from InputStream.read(). Cache key changed from command object to "tag:rawCommand" string for deterministic concurrent access.

Key improvements:

• Serialized command execution prevents stream corruption when multiple coroutines call run() concurrently
• Proper suspend function usage without blocking threads
• Fixed EOF handling for disconnect scenarios
• Added comprehensive tests for concurrency, caching, EOF, and cancellation
• Updated documentation with Android coroutine guidance and serialization behavior

Issues found:

• lateinit var rawData on line 48 can throw UninitializedPropertyAccessException if readRawData() throws before assignment
• Elapsed timing now includes artificial delayTime, changing semantics from previous version

Confidence Score: 4/5

• This PR is safe to merge with one critical issue that should be addressed
• The PR successfully fixes important coroutine anti-patterns (nested runBlocking), adds proper serialization for thread safety, and includes comprehensive tests. However, the lateinit var rawData on line 48 is a potential runtime error if readRawData() throws before assignment. The timing semantics change (including delayTime in elapsed time) is a behavioral change but not a bug. Overall solid refactoring with good test coverage.
• Pay close attention to ObdDeviceConnection.kt line 48 - the lateinit var could cause crashes if readRawData throws

Important Files Changed

Filename	Overview
src/main/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnection.kt	Removed nested runBlocking, added Mutex for serialization, fixed cache key and EOF handling, improved coroutine patterns
src/test/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnectionTest.kt	Added comprehensive tests for concurrent execution, cache safety, EOF handling, and cancellation propagation
README.md	Updated documentation to clarify coroutine usage, fix typos, and document serialization behavior

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Client calls run command] --> B{Acquire Mutex Lock}
    B --> C{Check Cache}
    C -->|Cache Hit & useCache=true| D[Return Cached Response]
    C -->|Cache Miss or useCache=false| E[runCommand]
    E --> F[sendCommand: Write to OutputStream]
    F --> G{delayTime > 0?}
    G -->|Yes| H[delay delayTime]
    G -->|No| I[readRawData]
    H --> I
    I --> J{inputStream.available > 0?}
    J -->|Yes| K{Read byte}
    K -->|byte == -1| L[EOF: Break]
    K -->|byte == '>'| M[End: Break]
    K -->|Other| N[Append to result]
    N --> J
    J -->|No| O{retriesCount <= maxRetries?}
    O -->|Yes| P[Increment retries, delay 500ms]
    P --> J
    O -->|No| Q[Return result]
    L --> Q
    M --> Q
    Q --> R{useCache=true?}
    R -->|Yes| S[Store in Cache with tag:rawCommand key]
    R -->|No| T[handleResponse]
    S --> T
    D --> U[Release Mutex Lock]
    T --> U
    U --> V[Return ObdResponse]

Loading

_{Last reviewed commit: fd19141}

[greptile-apps]

_{4 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Summary

This PR stabilizes coroutine execution in ObdDeviceConnection by removing nested runBlocking anti-patterns, serializing command execution with Mutex to prevent stream interleaving, and fixing EOF handling.

Key improvements:

• removed nested runBlocking from suspend paths (was blocking coroutine threads unnecessarily)
• added Mutex to serialize concurrent command execution (prevents stream interleaving)
• changed cache key from object reference to deterministic string (className:rawCommand)
• fixed EOF detection by properly checking InputStream.read() for -1
• made ioDispatcher injectable for testing
• moved delay() outside IO context (now properly suspends instead of blocking)

Tests added for concurrent execution, cache safety, EOF handling, and cancellation propagation.

Confidence Score: 4/5

• Safe to merge with minor consideration for the uninitialized variable pattern
• Core concurrency improvements are sound (Mutex serialization, removed nested runBlocking), EOF handling is correct, and comprehensive tests validate the changes. The uninitialized rawData variable relies on compiler flow analysis but compiles successfully.
• src/main/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnection.kt - verify the uninitialized variable pattern is intentional

Important Files Changed

Filename	Overview
src/main/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnection.kt	Removed nested runBlocking, added Mutex for serialization, fixed EOF handling, made cache deterministic with string keys
src/test/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnectionTest.kt	Added comprehensive tests for concurrent execution, cache safety, EOF handling, and cancellation propagation
README.md	Clarified Android coroutine usage and documented serialized connection behavior

Sequence Diagram

sequenceDiagram
    participant C1 as Coroutine 1
    participant C2 as Coroutine 2
    participant M as Mutex
    participant Conn as ObdDeviceConnection
    participant IO as InputStream/OutputStream

    C1->>Conn: run(SpeedCommand)
    Conn->>M: withLock
    M-->>Conn: lock acquired
    Conn->>IO: write("01 0D\r")
    IO-->>Conn: flush
    Conn->>Conn: delay(delayTime)
    Conn->>IO: read until '>' or EOF
    
    Note over C2,M: C2 attempts concurrent access
    C2->>Conn: run(RPMCommand)
    Conn->>M: withLock (waiting)
    Note over M: Mutex blocks C2 until C1 completes
    
    IO-->>Conn: "410D40>"
    Conn-->>M: release lock
    M-->>C1: ObdResponse
    
    M-->>Conn: lock acquired
    Conn->>IO: write("01 0C\r")
    IO-->>Conn: flush
    Conn->>Conn: delay(delayTime)
    Conn->>IO: read until '>' or EOF
    IO-->>Conn: "410C1AF8>"
    Conn-->>M: release lock
    M-->>C2: ObdResponse

Loading

_{Last reviewed commit: 623cfac}

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Summary

This PR successfully stabilizes coroutine execution in ObdDeviceConnection by replacing nested runBlocking calls with proper Mutex-based serialization. The changes improve thread safety, fix EOF handling, and make the cache deterministic by using stable cache keys.

Key improvements:

• Serialized command execution prevents stream interleaving via Mutex
• Proper coroutine integration with injected dispatcher for testability
• Fixed EOF detection by checking for -1 from InputStream.read()
• Deterministic cache keys using class name + raw command
• Comprehensive test coverage for concurrency, caching, EOF, and cancellation

Minor issue found: the retry loop condition uses <= instead of <, allowing one extra retry attempt beyond maxRetries (6 instead of 5 with default settings).

Confidence Score: 4/5

• This PR is safe to merge with the retry count logic issue being minor
• The changes are well-tested and address real concurrency issues. The off-by-one error in retry logic is the only functional bug found - it allows 6 retries instead of 5, which is harmless but should be corrected for accuracy
• Pay attention to ObdDeviceConnection.kt line 71 for the retry loop fix

Important Files Changed

Filename	Overview
src/main/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnection.kt	Replaces nested runBlocking with Mutex for serialized execution; adds injected dispatcher; improves EOF handling and cache key stability
src/test/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnectionTest.kt	New comprehensive tests for concurrency, caching, EOF handling, and cancellation; well-structured with helper classes

Sequence Diagram

sequenceDiagram
    participant C1 as Coroutine 1
    participant C2 as Coroutine 2
    participant M as Mutex
    participant ODC as ObdDeviceConnection
    participant Stream as Input/OutputStream
    
    C1->>+ODC: run(speedCommand)
    ODC->>+M: withLock
    M-->>ODC: lock acquired
    ODC->>ODC: check cache (miss)
    ODC->>Stream: write("01 0D\r")
    ODC->>Stream: flush()
    
    C2->>+ODC: run(rpmCommand)
    ODC->>M: withLock
    Note over M: C2 blocked waiting for mutex
    
    ODC->>Stream: read until '>' or EOF
    Stream-->>ODC: "410D40>"
    ODC-->>M: release lock
    ODC-->>-C1: ObdResponse
    
    M-->>ODC: lock acquired
    ODC->>ODC: check cache (miss)
    ODC->>Stream: write("01 0C\r")
    ODC->>Stream: flush()
    ODC->>Stream: read until '>' or EOF
    Stream-->>ODC: "410C1AF8>"
    ODC-->>M: release lock
    ODC-->>-C2: ObdResponse

Loading

_{Last reviewed commit: 8fe7a9a}

[greptile-apps]

_{3 files reviewed, 5 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

src/main/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnection.kt
initialized to empty string but if readRawData() throws before assignment on line 51, rawData will still contain "" rather than causing initialization error

        var rawData: String

then handle in try-catch or let exception propagate (preferred)

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

src/main/kotlin/com/github/eltonvs/obd/connection/ObdDeviceConnection.kt
rawData could remain uninitialized if readRawData() throws (e.g., IOException). Initialize to empty string.

        var rawData = ""