Repository navigation
refactor: split sendOneRequest into the steps of a request - #44
Merged
Merged
Conversation
ilyam8
added this pull request to stack #38
October 9, 2026 19:40
ilyam8
force-pushed
the
engine-exchange
branch
2 times, most recently
from
October 11, 2026 09:46
dd4e8d4 to
c54de9c
Compare
The request engine moves out of marshal.go unchanged, so the next commit's decomposition reads as a change within one file. marshal.go keeps the packet types, SafeString and MarshalMsg. No behavior change.
Mutation probes on the request engine's decomposition found that nothing
observed which version picks the decoding of a reply: the client's (today)
or the reply's.
- v3/answer/other-version/{noauth,md5,sha-aes}: an SNMPv3 client given an
SNMPv2c GetResponse with its request ID rejects it at every security
level and sends the request again.
- requests/answer/other-version-v3: an SNMPv2c client given SNMPv3
replies rejects them; the request fails with the PDU type error.
Both pass on the current code.
A mutation probe on the request engine's decomposition showed that no scenario told apart accepting the request IDs of all earlier attempts from accepting only the previous one's. requests/answer/late-by-two-attempts: the first request's reply arrives during the third attempt and answers the request. Passes on the current code.
sendOneRequest ran a request in one function: an attempt loop and a read loop, both labeled, steered by one err variable, so which failures retried and which returned depended on the loop a break left and on the value err held at that moment. It now reads as the steps of a request: - sendOneRequest: the attempt loop. It keeps the request IDs of earlier attempts and runs OnFinish when the request succeeds. - beforeRetry: OnRetry, then the request's error when no retry follows (a timeout under the context's deadline, the retries used up), else the next timeout. - attempt: context check, deadline, encode, hooks, write, then await. Its outcome is a value: the request's result, or a retry with its reason. - await: reads until a reply answers the request; a TCP EOF reconnects for the next attempt. - decode: header, then for SNMPv3 authentication and scoped PDU, then PDU. - answers: the empty-reply rule, Reports through a table of counters to errors, the request IDs. No behavior change: every golden is unchanged, and so is the logger output, checked against goldens regenerated with a transcript logger. Each known bug of the function keeps one site with a known-bug comment; the two timeout text matches share isTimeout. The request IDs of earlier attempts stay in sendOneRequest's frame, so the slice stays on the stack as before.
Review of the decomposition found pinned known bugs whose sites are in the new functions without a comment: - answers: nothing else of a reply is compared with the request (version, PDU type, msgID, and the security model, level, user, engine ID and context of RFC 3412 section 7.2 step 12 b), and a Report counts only with exactly one varbind, so one with more is returned as a successful reply. The reportErrors doc no longer states the one-varbind condition as what a Report is. - decode: after discovery a reply is checked with the client's flags, so an unauthenticated Report fails the digest check and is discarded; a reply with an empty msgFlags keeps the request's flags. attempt now stamps the request ID before encode, which keeps the per-message work (msg ID, privacy parameters, marshal); the IDs are taken in the same order. The stack note on the earlier request IDs holds for small Retries only, and says so. No behavior change: goldens and logger output unchanged.
An equivalence review of the request engine's decomposition found behaviors the engine tests did not observe; old and new code agree on each, and each pin passes on both. - describeEngineError adds the error's dynamic type and the sentinels it equals, so a returned error that gets wrapped (still errors.Is) or a context's cause returned instead of its error shows in the goldens. - requests/context/canceled-with-cause-before: a context canceled with a cause gives context.Canceled. - requests/context/deadline-equal-to-timeout: a context deadline equal to the attempt's ends it as a request timeout. - requests/answer/other-version-v3-report: an SNMPv3 Report whose msgData is the bare PDU, sent to an SNMPv2c client, maps to its error. - v3/hooks/privacy-protocol-changed-on-retry: an OnRetry hook that changes the client's privacy protocol makes the next attempt fail in initPacket. - TestEngineTCPReconnect: retries that run out after a timeout and then a reconnect end with "max retries" (the reconnect leaves no error); a reply to the first request read after the reconnect answers it. - TestEngineReplyLogger: a reply carries the client's Logger.
Only an empty reply without an error status answers early; one with an error status goes through the request ID check.
ilyam8
force-pushed
the
engine-exchange
branch
from
October 11, 2026 09:52
c54de9c to
51f9024
Compare
vkalintiris
approved these changes
Oct 11, 2026
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
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.
Summary
Splits
sendOneRequest, the core of the request engine, into the steps of a request. No behavior or exported API change.sendOneRequestused to run a request in one function: an attempt loop and a read loop, both labeled, steered by one sharederr. Which failures retried and which ended the request depended on the loop abreakleft and on the valueerrheld at that moment. It now reads as:sendOneRequest: the attempt loop; keeps the request IDs of earlier attempts and runs OnFinish on success.beforeRetry: OnRetry, then the request's error when no retry follows (a timeout under the context's deadline, the retries used up), else the next timeout.attempt: context check, deadline, request ID, encode, hooks, write, thenawait. Its outcome is a value: the request's result, or a retry with its reason.await: reads until a reply answers the request; a TCP EOF reconnects for the next attempt.decode: header, then for an SNMPv3 client authentication and scoped PDU, then PDU.answers: the empty-reply rule, Reports through a table of counters to errors, the request IDs.The first commit moves
sendandsendOneRequestfrommarshal.gointoengine.gounchanged, so the decomposition reads as a change within one file;senditself is unchanged (next step).Each known bug of the old function keeps one site with a known-bug comment. The request IDs of earlier attempts stay in
sendOneRequest's frame, so the slice stays on the stack as before.The test commits pin behaviors mutation probes and review found unobserved; each pin passes on the old code: replies of another SNMP version than the client's, a reply to the first attempt read in the third, the dynamic type and sentinel identity of every returned error (
describeEngineError), a context canceled with a cause, a context deadline equal to the attempt's, a hook that changes the privacy protocol between attempts, two TCP reconnect paths, and the reply'sLogger.Testing
go test ./...on darwin/arm64 andGOARCH=386;-race; the FIPS 140-only tests; golangci-lint v2.14.0 (incl.end2end): 0 issues;GOOS=windows go vet.engine.go: 96 caught by the suite; 12 change only logger output (11 caught by the logger comparison; the 12th drops a line reachable only over loopback TCP); 1 differs only when a hook rewrites the client's engine ID mid-request; 1 skipped (superseded by request-ID order probes, caught).BenchmarkSendOneRequest, interleaved A/B: B/op and allocs/op identical; time within noise in 6- and 12-round runs (a 12x12 run measured +0.7%, about 7 ns per request).