Skip to content

Fix race conditions in the SSE and WS code#414

Merged
mathieucarbou merged 1 commit intomainfrom
fix/sse
Mar 18, 2026
Merged

Fix race conditions in the SSE and WS code#414
mathieucarbou merged 1 commit intomainfrom
fix/sse

Conversation

@mathieucarbou
Copy link
Member

@mathieucarbou mathieucarbou commented Mar 18, 2026

No description provided.

@mathieucarbou mathieucarbou requested review from Copilot, me-no-dev and willmmiles and removed request for Copilot March 18, 2026 11:13
@mathieucarbou mathieucarbou self-assigned this Mar 18, 2026
Copilot AI review requested due to automatic review settings March 18, 2026 11:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a race condition in the Server-Sent Events (SSE) message queuing path by ensuring the queue overflow check happens after acquiring the ESP32 message-queue lock.

Changes:

  • Moved _messageQueue.size() overflow checks to occur after _lockmq is acquired (ESP32 builds).
  • Applied the same ordering fix to both _queueMessage overloads.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mathieucarbou mathieucarbou changed the title Fix race condition in the SSE code Fix race conditions in the SSE and WS code Mar 18, 2026
@mathieucarbou mathieucarbou merged commit 2a3c634 into main Mar 18, 2026
33 checks passed
@mathieucarbou mathieucarbou deleted the fix/sse branch March 18, 2026 12:36
#ifdef ESP32
std::lock_guard<std::recursive_mutex> lock(_client_queue_lock);
#endif
if (_disconnectcb) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change may be incorrect. Client callbacks shouldn't be called while holding the lock -- if the client does something like "hmm, I'm done with this server now, lets destruct it" we'll have a bad time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same issue existed here:

void AsyncEventSource::_addClient(AsyncEventSourceClient *client) {
if (!client) {
return;
}
#ifdef ESP32
std::lock_guardstd::recursive_mutex lock(_client_queue_lock);
#endif
_clients.emplace_back(client);
if (_connectcb) {
_connectcb(client);
}

_adjust_inflight_window();
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added 2 commits in #415 to fix the 2 situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants