TCP link recovery reconnects in a tight loop with no retry interval

[hnicolaysen]

Problem

tcpTransporter.Send's readResultCloseRetry branch closes the connection and goes straight back round the loop with no delay:

case readResultCloseRetry:
	mb.logf("modbus: close connection and retry reading response, because of %v", err)
	mb.close()
	select {
	case <-ctx.Done():
		return nil, ctx.Err()
	default:
		continue
	}

Each iteration is a full TCP dial plus a request write plus a read. Against a device that accepts a connection and drops it without answering, this spins as fast as the network allows for the whole LinkRecoveryTimeout budget.

Measured with a server that reads the request and closes immediately, LinkRecoveryTimeout = 500ms:

transaction IDs on the wire: 1 (x 15726 within the 500ms budget)
unique ids: ['1']

~30k dials/second at a single embedded Modbus server. These are small microcontrollers — that is enough to wedge one, and the reconnect storm can outlast the condition that triggered it.

This is also the behaviour the spec singles out. MODBUS Messaging on TCP/IP Implementation Guide V1.0b §4.4.1.4:

any 'timeout' time used at a client to initiate an application retry should be larger than the expected maximum 'reasonable' response time. If this is not followed, there is a potential for excessive congestion at the target device or on the network, which may in turn cause further errors. This is a characteristic, which should always be avoided.

The serial transporter already solves this

ef3bed5 ("Add retry interval for serial connections to avoid link flapping") gave serialPort.reconnect() a ticker-paced retry loop:

• ReconnectRetryInterval time.Duration on the transporter (serial.go:40)
• serialReconnectRetryInterval = 10 * time.Millisecond as the default (serial.go:23)
• reconnectRetryInterval() accessor falling back to the default (serial.go:141-144)
• a time.NewTicker(...) the reconnect loop selects on, alongside the deadline and ctx.Done()

The TCP path never got the equivalent, so the two transports behave very differently under the same fault.

Suggested fix

Port that pattern to tcpTransporter: add ReconnectRetryInterval with a default, and have the readResultCloseRetry branch select on a ticker as well as ctx.Done(). Keeping the default small (the serial one is 10 ms) preserves fast recovery for the case link recovery exists for, while bounding the loop to something a device can absorb.

Context

Found while reviewing #133, which removed a different duplicate-request path in the same function and gated this branch so that writes are no longer reissued here. Reads still are, so the tight loop remains reachable. Not fixed there to keep that PR to one behavioural change.

[hnicolaysen]

Two further measurements from reviewing #133, both strengthening the case.

Higher budget, proportionally worse. With LinkRecoveryTimeout = 2s against a listener that accepts and immediately closes: 32,837 dial+write+read cycles in 2.0s — ~16k connections/second, consistent with the 15,726-in-500ms figure above. It scales linearly with the budget, so raising LinkRecoveryTimeout to tolerate a slower device makes the storm proportionally larger.

The transport mutex is held throughout. Send takes mb.mu for its whole duration, so the entire recovery budget blocks every other request to that device — including requests for other unit IDs sharing the connection through a gateway, which is the multiplexing case in #56. So the cost is not only the reconnect storm at the device, it is a full stall of the client for that address.

Neither is introduced by #133 — both are properties of the pre-existing loop — but #133 gated this branch so that only reads reach it, which means what remains is a read being reissued up to ~16k times per second. A retry interval is the missing piece.