Replace string-matched netlink timeout detection with net.Error #65

Merged

danny merged 1 commit from issue16_error_handling into main

2026-07-05 19:05:17 +01:00

danny commented

2026-07-05 18:52:28 +01:00

(Migrated from gitlab.keyop.co.uk)

Closes #16 (https://gitlab.keyop.co.uk/keyop/go/nfq_forwarder/-/issues/16).

Background

nfErrorFunction is registered as go-nfqueue's error callback via RegisterWithErrorFunc, against initNfqueue's nfqueue.Config{ReadTimeout: 10 * time.Millisecond, ...}. That read timeout is how go-nfqueue's internal read loop periodically wakes up to check for context cancellation - so nfErrorFunction is called roughly every 10ms whenever no packet is waiting on the queue, confirming the "we may get called a lot" comment already on the ticket. It needs to correctly and cheaply recognise "this is just the routine timeout wakeup" on every one of those calls.

The existing implementation did that by comparing err.Error() against two hardcoded strings ("netlink receive: i/o timeout" / "netlink receive: recvmsg: i/o timeout") - fragile, since that formatted text is an implementation detail of mdlayher/netlink.OpError's Error() method, not a documented contract, and has already taken two different shapes depending on exactly how the underlying read failed.

Fix

mdlayher/netlink.OpError (the actual concrete error type Conn.Receive() returns) already implements the standard net.Error interface, including a correct Timeout() bool. Replaced the string comparison with:

var netErr net.Error
if errors.As(err, &netErr) && netErr.Timeout() {
    return 0
}

This is robust to message-text changes across dependency versions (checks behavior/type, not formatted text), and - unlike the string comparison - also correctly recognises a timeout even if some future code wraps the error further, since errors.As walks the full Unwrap() chain.

No behavior change for real production traffic - this is purely a correctness/robustness fix for how "is this a benign read-timeout" is detected.

Testing

TestNfErrorFunction now constructs a real *netlink.OpError wrapping a real timeout-capable error (os.ErrDeadlineExceeded) instead of a plain string, plus that same error wrapped an extra level deeper via fmt.Errorf("...: %w", ...) - a genuine regression case the old implementation got wrong (the wrapped error's formatted text no longer matches either hardcoded string, so it would have been incorrectly logged as a real error every ~10ms). The plain non-timeout "still logs" case is unchanged.

go build ./..., go vet ./...
make lint (golangci-lint + yamllint) - go mod tidy promoted mdlayher/netlink from an indirect to a direct dependency, since the test now imports it directly
make test (go test -race -cover ./...)

Docs

Updated CLAUDE.md's Design notes and Testing sections.

Closes #16 (https://gitlab.keyop.co.uk/keyop/go/nfq_forwarder/-/issues/16). ## Background `nfErrorFunction` is registered as `go-nfqueue`'s error callback via `RegisterWithErrorFunc`, against `initNfqueue`'s `nfqueue.Config{ReadTimeout: 10 * time.Millisecond, ...}`. That read timeout is how `go-nfqueue`'s internal read loop periodically wakes up to check for context cancellation - so `nfErrorFunction` is called **roughly every 10ms whenever no packet is waiting on the queue**, confirming the "we may get called a lot" comment already on the ticket. It needs to correctly and cheaply recognise "this is just the routine timeout wakeup" on every one of those calls. The existing implementation did that by comparing `err.Error()` against two hardcoded strings (`"netlink receive: i/o timeout"` / `"netlink receive: recvmsg: i/o timeout"`) - fragile, since that formatted text is an implementation detail of `mdlayher/netlink.OpError`'s `Error()` method, not a documented contract, and has already taken two different shapes depending on exactly how the underlying read failed. ## Fix `mdlayher/netlink.OpError` (the actual concrete error type `Conn.Receive()` returns) already implements the standard `net.Error` interface, including a correct `Timeout() bool`. Replaced the string comparison with: ```go var netErr net.Error if errors.As(err, &netErr) && netErr.Timeout() { return 0 } ``` This is robust to message-text changes across dependency versions (checks behavior/type, not formatted text), and - unlike the string comparison - also correctly recognises a timeout even if some future code wraps the error further, since `errors.As` walks the full `Unwrap()` chain. No behavior change for real production traffic - this is purely a correctness/robustness fix for how "is this a benign read-timeout" is detected. ## Testing `TestNfErrorFunction` now constructs a real `*netlink.OpError` wrapping a real timeout-capable error (`os.ErrDeadlineExceeded`) instead of a plain string, plus that same error wrapped an extra level deeper via `fmt.Errorf("...: %w", ...)` - a genuine regression case the _old_ implementation got wrong (the wrapped error's formatted text no longer matches either hardcoded string, so it would have been incorrectly logged as a real error every \~10ms). The plain non-timeout "still logs" case is unchanged. - [x] `go build ./...`, `go vet ./...` - [x] `make lint` (golangci-lint + yamllint) - `go mod tidy` promoted `mdlayher/netlink` from an indirect to a direct dependency, since the test now imports it directly - [x] `make test` (`go test -race -cover ./...`) ## Docs Updated `CLAUDE.md`'s Design notes and Testing sections.

danny (Migrated from gitlab.keyop.co.uk) approved these changes

2026-07-05 18:52:28 +01:00

danny commented

2026-07-05 19:02:30 +01:00

(Migrated from gitlab.keyop.co.uk)

changed the description

danny commented

2026-07-05 19:02:30 +01:00

(Migrated from gitlab.keyop.co.uk)

requested review from @danny