modbus: limit link error-recovery sleep+flush to confirmations

[JacksongXu]

Limit MODBUS_ERROR_RECOVERY_LINK's sleep+flush to MSG_CONFIRMATION

Symptom

Running a server (both RTU and TCP backends tested) on a non-blocking,
select()-driven receive loop with MODBUS_ERROR_RECOVERY_LINK
enabled, client requests would occasionally go completely unanswered —
no error surfaced on either side, just a missing response. See
"Testing" below for how this was traced back to
_modbus_receive_msg().

Root cause

In _modbus_receive_msg(), when select() times out (errno == ETIMEDOUT)
and MODBUS_ERROR_RECOVERY_LINK is enabled, the library unconditionally
runs _sleep_response_timeout() followed by modbus_flush():

if (errno == ETIMEDOUT) {
    _sleep_response_timeout(ctx);
    modbus_flush(ctx);
} else if (errno == EBADF) {
    modbus_close(ctx);
    modbus_connect(ctx);
}

This runs for both msg_type values — MSG_CONFIRMATION (a client
waiting for a response) and MSG_INDICATION (a server/slave waiting for
the next request). That's a problem specifically for the indication case:

• _sleep_response_timeout() blocks synchronously for up to
  response_timeout. A server built around a non-blocking,
  select()-driven event loop (servicing an RTU bus alongside other
  fds/timers) gets stalled for that whole window, which defeats the
  purpose of a non-blocking design.
• modbus_flush() then discards the OS receive buffer. On a shared
  RS485 bus, if the master already started sending a new request while
  the server was sleeping, that request is silently dropped instead of
  being picked up on the next loop iteration.

A timeout while waiting for an indication is routine, expected behavior
for a server that simply has no traffic yet. It isn't a link-layer
failure that warrants a reconnect-style sleep/flush sequence. That
sequence makes sense for a client that just sent a request and got no
response: sleeping for response_timeout and flushing stale bytes
before retrying is a reasonable recovery there.

The docs already flag this indirectly:

It's not recommended to enable error recovery for slave/server.

This change is meant to make that recommendation unnecessary, rather
than something users have to remember.

Change

Scope the sleep+flush branch to MSG_CONFIRMATION only:

if (errno == ETIMEDOUT && msg_type == MSG_CONFIRMATION) {
    _sleep_response_timeout(ctx);
    modbus_flush(ctx);
} else if (errno == EBADF) {
    modbus_close(ctx);
    modbus_connect(ctx);
}

EBADF handling is left unconditional — a bad file descriptor is a
genuine link failure regardless of client/server role.

Compatibility

• Default error_recovery is MODBUS_ERROR_RECOVERY_NONE, so this is a
  no-op for anyone not already opting into
  MODBUS_ERROR_RECOVERY_LINK.
• For existing MODBUS_ERROR_RECOVERY_LINK client users
  (MSG_CONFIRMATION path), behavior is unchanged.
• For server/slave users, this only removes a stall+drop behavior that
  wasn't documented as intentional and was already discouraged in the
  docs — it doesn't change what a caller receives from modbus_receive()
  on timeout (-1 / errno == ETIMEDOUT, same as before).

Minimal reproduction

This reproduces reliably, not just intermittently under load, given:

• Server: response_timeout = 500ms (library default), indication_timeout
  explicitly set to 1s (default is unset, which blocks indefinitely —
  see note below on why this matters), MODBUS_ERROR_RECOVERY_LINK
  enabled, non-blocking select()-driven receive loop.
• Client: sends requests at arbitrary/uncoordinated times relative to the
  server's idle cycle — no special timing needed on the client side.

Root cause of why this reproduces so easily: _sleep_response_timeout()
always sleeps for ctx->response_timeout, regardless of which timeout
actually fired the ETIMEDOUT:

static void _sleep_response_timeout(modbus_t *ctx) {
    /* Response timeout is always positive */
    ...
    request.tv_sec = ctx->response_timeout.tv_sec;
    request.tv_nsec = ((long int) ctx->response_timeout.tv_usec) * 1000;
    ...
}

With the settings above: every ~1s of idle time, the server's indication
wait times out (indication_timeout expiring is the whole point of
setting it explicitly for a non-blocking design — it's what lets the
event loop come back around instead of blocking forever). That timeout
then triggers a 500ms sleep (response_timeout, unrelated to the 1s
that actually elapsed) followed by a flush. Any request the client sends
during that ~500ms window — which recurs every idle cycle — lands in the
OS receive buffer sometime before or during the flush and is discarded.
With uncoordinated client timing, the probability of a collision per
idle cycle is roughly sleep_duration / cycle_length (≈500ms / ~1.5s
here), which is high enough to show up quickly in normal operation, not
just as a rare race.

Note: this manifests when indication_timeout is explicitly set to
a finite value. With the library default (indication_timeout unset,
select() blocks indefinitely waiting for a request), the indication
wait itself never times out, so this path isn't triggered. Setting
indication_timeout explicitly is precisely what a non-blocking,
select()-driven server needs to do to stay responsive to other
fds/timers in its event loop instead of blocking forever on one read,
which is why this shows up for exactly the kind of design libmodbus is
otherwise well-suited to.

Testing

This fix was arrived at empirically, not just by code inspection — see
"Symptom" above for how the drops were first observed.

• Manual: server (both RTU and TCP backends) driven by a
  non-blocking select() loop, MODBUS_ERROR_RECOVERY_LINK
  enabled, concurrent client traffic. Before this change: client
  requests are intermittently dropped by the server-side
  modbus_flush() racing with in-flight request bytes. After this
  change: no drops observed under the same traffic pattern, and
  modbus_receive() still returns promptly (-1 /
  errno == ETIMEDOUT) on genuine idle timeouts, with no sleep or
  flush performed on the indication path.

Notes

Happy to adjust scope/wording or add a test per maintainer preference —
this is a small, deliberately minimal change to keep review easy.

[cla-bot]

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...