Skip to content

Request write is outside the response timeout, leaving requests unbounded and blocking reconnect #195

Description

@jadamcrain

ClientLoop::execute_request formats the request and then awaits the write before computing the deadline:

io.write(bytes, self.decode.physical).await?;

let deadline = Instant::now() + request.timeout;

rodbus/src/client/task.rs:273

Because deadline is computed after the write returns, the write is not covered by RequestParam::response_timeout. A peer that stops reading — e.g. a closed TCP receive window, where zero-window probes keep the connection alive indefinitely — parks the loop in io.write with no bound. A caller awaiting read_coils() with a 5s response timeout can wait forever.

The recovery path can't fire either. RequestError::ResponseTimeout is never produced, so timeout_counter never increments and max_timeouts never tears the session down to reconnect. The mechanism that exists specifically to recover from unresponsive peers is unreachable in exactly the case it was built for, and the channel stays wedged with no self-healing.

Fix

Move the deadline above the write and bound the write with it:

let deadline = Instant::now() + request.timeout;

tokio::select! {
    _ = tokio::time::sleep_until(deadline) => return Err(RequestError::ResponseTimeout),
    res = io.write(bytes, self.decode.physical) => res?,
}

The write timeout must be session-fatal. write_all is not transactionally cancel-safe: cancelling it may leave a partial ADU on the wire, and the peer will interpret the next request's bytes as a continuation of the truncated frame. So this cannot follow the existing non-fatal ResponseTimeout path that keeps the connection and moves to the next request — it has to drop and re-establish the transport (TCP/TLS) or close and reopen the port (RTU).

That distinction is worth making explicit in the code, since ResponseTimeout currently means "connection is fine, this request didn't answer in time" and a write timeout means something materially different.

Notes

Independent of #196, though they interact: the wedged write is also the reason shutdown can hang forever today, and the out-of-band signal proposed there interrupts it from the outside. This issue is about a request having no time bound at all, which is a problem whether or not shutdown exists.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions