TCP connection error handling logic

[sansmoraxz]

I am trying to understand the link recovery mechanism. So for this, I am using a tcp modbus simulator. Sometimes I restart the server, or disconnect/ reconnect the simulator into the network.

So here's what I have observed. io.EOF is being propagated into the clients.

So, is connection recovery a built in feature of the library, or should be handle those from client side (by dropping the previous client).

Going deeper into the codebase, I see this:

modbus/tcpclient.go

Lines 321 to 328 in 99e372e

	} else if (err != io.EOF && err != io.ErrUnexpectedEOF) ||
	mb.LinkRecoveryTimeout == 0 || time.Until(recoveryDeadline) < 0 {
	return
	}
	// any other error, but recovery deadline isn't reached yet - close and retry
	res = readResultCloseRetry
	return
	}

The condition (err != io.EOF && err != io.ErrUnexpectedEOF) check feels convoluted. Also, readResultCloseRetry is not used anywhere else other than this assignment.

Feels like a logic bug, but I am not sure of the intention here too.

[sansmoraxz]

Also the label for IdleTimeout is that cached connections never time out for negative values. But the deadline is set by adding IdleTimeout value to request time. This will always be a past value, ie deadline check expire will always pass.

[andig]

This fits my observations (can‘t find the comment atm). Link recovery doesn‘t do what it is supposedly designed to do.

[antoineco]

This part of the code has admittedly grown into a spaghetti. One of the timeouts (I think the one you mentioned) is not even applied anywhere and only compared to 0 if I remember correctly.

I am lacking the time to address this at the moment, but PRs are welcome. @andig even did all the work in his own fork already in case you want to copy-paste.

[andig]

I didn't entirely unfortunately. Basically worked around by adding client-side retries. Thank you for this PR!