fix(transport): bump default request-timeout from 2s to 10s#15
Merged
Conversation
The default 2s was tight enough that END_CHANGE_DB (the flash-commit trigger fired at the end of a write transaction) routinely timed out on non-trivial imports, even though the device actually finished the commit successfully. The user-visible result was a non-zero exit code plus a confusing "timed out" message immediately after the operation the user wanted had in fact landed. Calibration: the reporter confirmed 30s as a value that's clearly sufficient and 2s as one that isn't; 10s is the middle ground that preserves a useful fast-fail signal for actually-stuck connections while covering typical commits with comfortable margin. Users performing very large transactions still have --request-timeout (or MYTHY_REQUEST_TIMEOUT) to raise further. This is a global bump rather than a per-operation override because the underlying simonvetter/modbus client doesn't expose a setter for the request timeout post-construction, and recreating the client just to widen one round-trip would be heavier than the problem warrants. Closes #7 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7.
What changed
--request-timeoutdefault goes from 2s to 10s. Same change in three places (TCP transport, RTU transport, CLI flag) plus the README table. Help-string wording unchanged — terse style matches surrounding flags.Why 10s and not 30s
The issue mentions 30s "makes the problem go away" but that's a confirmed-OK value, not a measured minimum. 10s sits comfortably above the 2s failure point while preserving a useful fast-fail signal for genuinely stuck connections — falling back from "why is this hanging" diagnostics from 2s to 30s would be a regression for the routine operator. Users running unusually large imports can still raise via
--request-timeoutorMYTHY_REQUEST_TIMEOUT.Why a global bump and not a per-operation override
The cleaner shape would be: keep
--request-timeoutshort and add a separate--commit-timeoutconsulted only insideEdit.Commit. I looked at this; the underlyingsimonvetter/modbusclient has no public setter for its request timeout after construction (SetTimeout/SetRequestTimeoutetc. don't exist on theClientinterface, and the field is initialised atOpentime). The two alternatives — re-opening the client around the commit, or vendoring/forking the library — both cost more than the bug justifies. Banking the global bump and keeping per-operation overrides as a follow-up if 10s ever stops being enough.Tests
No new tests. The change is a constant in three locations; behaviour-level coverage would have to assert on the modbus library's internal timer, which isn't observable from outside.
🤖 Generated with Claude Code