Spun off from Qodo's review of #185.
Problem
DaqifiDevice.ExecuteTextCommandAsync mutates shared device state for the duration of each call:
- Unsubscribes/resubscribes the protobuf consumer's
MessageReceived handler
- Stops and restarts the shared
_messageConsumer
- Creates a temporary
StreamMessageConsumer<string> on the same underlying transport stream
- Mutates the stream's
ReadTimeout
There is no synchronization around any of this. If two callers enter ExecuteTextCommandAsync concurrently they can corrupt each other's consumer lifecycle and stream reads — the second caller can stop a consumer the first caller is mid-restart on, two text consumers can read from the same stream, the read-timeout restore in finally can clobber a value the other caller just set, etc.
Multiple existing methods rely on ExecuteTextCommandAsync (InitializeAsync, GetSdCardFilesAsync, DeleteSdCardFilesAsync, DownloadSdCardFileAsync, the new DrainErrorQueueAsync, plus DaqifiStreamingDevice SD-card paths). Today the constraint that callers must not invoke them concurrently is implicit; nothing enforces it.
Proposal
Add a private SemaphoreSlim(1, 1) on DaqifiDevice and acquire it for the duration of ExecuteTextCommandAsync:
- Acquire at the top, observing
cancellationToken (await _textExchangeLock.WaitAsync(cancellationToken))
- Release in a
finally so the consumer-lifecycle cleanup still runs even if a caller cancels
- Behavior otherwise unchanged
This serializes every text exchange device-wide, removing the foot-gun without changing any public contract. Callers that already serialize naturally (e.g. through awaiting) see no behavior change; callers that would have raced now wait their turn.
Refs
Spun off from Qodo's review of #185.
Problem
DaqifiDevice.ExecuteTextCommandAsyncmutates shared device state for the duration of each call:MessageReceivedhandler_messageConsumerStreamMessageConsumer<string>on the same underlying transport streamReadTimeoutThere is no synchronization around any of this. If two callers enter
ExecuteTextCommandAsyncconcurrently they can corrupt each other's consumer lifecycle and stream reads — the second caller can stop a consumer the first caller is mid-restart on, two text consumers can read from the same stream, the read-timeout restore infinallycan clobber a value the other caller just set, etc.Multiple existing methods rely on
ExecuteTextCommandAsync(InitializeAsync,GetSdCardFilesAsync,DeleteSdCardFilesAsync,DownloadSdCardFileAsync, the newDrainErrorQueueAsync, plusDaqifiStreamingDeviceSD-card paths). Today the constraint that callers must not invoke them concurrently is implicit; nothing enforces it.Proposal
Add a private
SemaphoreSlim(1, 1)onDaqifiDeviceand acquire it for the duration ofExecuteTextCommandAsync:cancellationToken(await _textExchangeLock.WaitAsync(cancellationToken))finallyso the consumer-lifecycle cleanup still runs even if a caller cancelsThis serializes every text exchange device-wide, removing the foot-gun without changing any public contract. Callers that already serialize naturally (e.g. through awaiting) see no behavior change; callers that would have raced now wait their turn.
Refs
src/Daqifi.Core/Device/DaqifiDevice.cs—ExecuteTextCommandAsynclines 319–471