Skip to content

Commit

Permalink
Fixes race condition in ESP32 select-wakeup implementation.
Browse files Browse the repository at this point in the history
A correct implementation of a selectable fd driver has to check in vfs_select_start()
whether the fd is readable. This check was missing from the prior implementation.
We had an application level check of the queue being non-empty, but there is a
window of time between the application doing this check, and the select()
implementation of the esp32 getting to calling vfs_select_start(). The wakeup
implementation was only effective if it came after vfs_select_start, by the design
of esp's select mechanism, since the wakeup semaphore only comes in vfs_select_start.

Since the esp32's select() is very slow, this was actually a pretty big gap.

The OpenMRN Device::select implementation does not suffer from this race
condition, because the event group bits can be set at any time, even if
Device::select is still in the setup phase. Added a comment to this effect.
  • Loading branch information
balazsracz committed Feb 3, 2024
1 parent 20435f9 commit da3ee31
Showing 1 changed file with 14 additions and 0 deletions.
14 changes: 14 additions & 0 deletions src/os/OSSelectWakeup.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ int OSSelectWakeup::select(int nfds, fd_set *readfds,
else
{
#if OPENMRN_FEATURE_DEVICE_SELECT
// It is important that we do this select_clear() in the same
// critical section as checking pendingWakeup above. This ensures
// tht all wakeups before this clear cause sleep to be zero, and
// all wakeups after this clear will cause the event group bit to
// be set.
Device::select_clear();
#endif
}
Expand Down Expand Up @@ -189,6 +194,15 @@ void OSSelectWakeup::esp_start_select(fd_set *readfds, fd_set *writefds,
exceptFds_ = exceptfds;
exceptFdsOrig_ = *exceptfds;
FD_ZERO(exceptFds_);
if (pendingWakeup_)
{
// There is a race condition between the Executor deciding to run
// select, and the internal implementation of select() calling this
// function. Since we only get the semaphone in this call, the wakeup
// functions are noops if they hit during this window. If there was a
// missed wakeup, we repeat it.
esp_wakeup();
}
}

/// This function marks the stored semaphore as invalid which indicates no
Expand Down

0 comments on commit da3ee31

Please sign in to comment.