Fix transport deadlocks and improve test reliability#7
Merged
wizzomafizzo merged 5 commits intomainfrom Aug 15, 2025
Merged
Conversation
- Introduce comprehensive tests for concurrency and resource cleanup - Improve context propagation and cancellation in transport code - Refactor UART transport to prevent deadlocks and enhance reliability - Add context-aware methods for improved validation and safety - Enhance NDEF message validation with buffer safety checks - Implement backoff mechanism in waitAck to avoid CPU spinning
…prevention - Introduce comprehensive transport testing helpers (`BlockingMockTransport`) - Add tests for deadlock scenarios, context cancellation, and concurrent operations - Refactor `mockHangingTransport` for enhanced reliability and resource cleanup - Improve `waitAck` logic with structured state management and timeout handling - Enhance UART transport tests with context-aware checks for connection safety
The BlockingMockTransport was implementing SetTimeout as a no-op, which caused context cancellation tests to fail intermittently. The transport would block indefinitely on the blockChan, ignoring timeout settings from the transportContextAdapter. Changes: - Add timeout field to BlockingMockTransport struct - Implement proper timeout handling in SetTimeout method - Update SendCommand to use select with time.After for timeout support - Return appropriate timeout errors when operations exceed the timeout This fixes the failing TestSendCommandContextCancellationBehavior test by ensuring that context cancellation works properly through the timeout mechanism. Fixes the intermittent CI failures in deadlock detection tests.
- Fix goroutine leaks by setting short timeouts on BlockingMockTransport instances - Increase cleanup timeout in transport_context.go from 10ms to 20ms - Adjust timing expectations in tests to account for cleanup mechanism overhead - Remove redundant os.Exit call from TestMain to comply with Go 1.15+ best practices - Remove unused "os" import after TestMain fix All deadlock tests now pass with goleak enabled, ensuring proper goroutine cleanup without false positives from timing-sensitive validation.
Remove over-engineered testing infrastructure that was causing maintenance burden: - Remove deadlock_fixes_test.go (383 lines of artificial stress tests) - Remove validation_context_test.go (75 lines, minimal value) - Simplify transport_context_test.go from 281 to 62 lines (basic context test only) - Replace complex BlockingMockTransport with simple SimpleMockTransport - Remove goleak dependency and TestMain integration - Remove test-deadlock Makefile target The production code in transport_context.go remains unchanged and handles goroutine cleanup properly. Tests now focus on actual NFC functionality rather than infrastructure edge cases, resulting in: - Much faster test execution - Cleaner test output - Better maintainability - 3:1 production-to-test ratio instead of 1:3.5 All tests pass cleanly without timing-sensitive assertions or artificial concurrency scenarios.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Quality Improvements
time.Sleep()with polling loopsTechnical Changes
Test Plan