`nfErrorFunction` does string matching to determine error type

danny commented

2024-07-16 22:24:54 +01:00

(Migrated from gitlab.keyop.co.uk)

There has to be a better way than this:

func nfErrorFunction(err error) int {
	// There has to be a better way to do this. Raising issue
	if err.Error() == "netlink receive: i/o timeout" ||
		err.Error() == "netlink receive: recvmsg: i/o timeout" {
		// This appears to be normal, ignore
		return 0
	}
	log.Info("Error received:", err)
	return 0
}

This is used in RegisterWithErrorFunc - it's a little hard to test, as I don't know that our error function has ever actually been called.

There has to be a better way than this: ``` func nfErrorFunction(err error) int { // There has to be a better way to do this. Raising issue if err.Error() == "netlink receive: i/o timeout" || err.Error() == "netlink receive: recvmsg: i/o timeout" { // This appears to be normal, ignore return 0 } log.Info("Error received:", err) return 0 } ``` This is used in `RegisterWithErrorFunc` - it's a little hard to test, as I don't know that our error function has ever actually been called.

danny commented

2024-07-16 22:25:49 +01:00

(Migrated from gitlab.keyop.co.uk)

Actually, considering what this is doing, we may get called a lot.

danny commented

2026-07-05 18:48:41 +01:00

(Migrated from gitlab.keyop.co.uk)

Still valid - and it matters more than it looks

nfErrorFunction is registered 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 - which
means nfErrorFunction is called roughly every 10ms whenever no packet is waiting on the
queue, confirming the "we may get called a lot" comment on this ticket. It needs to correctly
and cheaply recognise "this is just the routine timeout wakeup" on every one of those calls.

The current string-matching approach is fragile for exactly the reason the original comment
suspected: the message text ("netlink receive: i/o timeout" / "netlink receive: recvmsg: i/o
timeout") is an implementation detail of mdlayher/netlink's OpError.Error() formatting, not
a documented contract - it's already two different strings depending on exactly how the
underlying read failed, and any future dependency bump could change it again and silently start
logging a message on every single timeout tick.

Plan

mdlayher/netlink.OpError (the actual concrete error type Conn.Receive() returns) already
implements the standard net.Error interface, including a correct Timeout() bool that
unwraps through *os.SyscallError where needed. Replace the string comparison with the
idiomatic, documented Go approach:

var netErr net.Error
if errors.As(err, &netErr) && netErr.Timeout() {
    // Routine read-timeout wakeup - not a real error, ignore.
    return 0
}

This is robust to message-text changes across library versions (it 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 (errors.As walks the full Unwrap() chain).

The existing test (TestNfErrorFunction) currently constructs plain errors.New("netlink receive: i/o timeout") strings, which only exercises the current buggy approach faithfully -
it'll be updated to construct a real *netlink.OpError wrapping a real timeout-capable error
(proving detection works against the actual type this function receives in production, not just
a string that happens to match today), plus a case wrapped an extra level deeper (to prove the
new check's advantage over string matching), and a plain non-timeout error (unchanged "still
logs" case).

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

Work is happening on branch issue16_error_handling; an MR will follow referencing this issue.

## Still valid - and it matters more than it looks `nfErrorFunction` is registered 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 - which means **`nfErrorFunction` is called roughly every 10ms whenever no packet is waiting on the queue**, confirming the "we may get called a lot" comment on this ticket. It needs to correctly and cheaply recognise "this is just the routine timeout wakeup" on every one of those calls. The current string-matching approach is fragile for exactly the reason the original comment suspected: the message text ("netlink receive: i/o timeout" / "netlink receive: recvmsg: i/o timeout") is an implementation detail of `mdlayher/netlink`'s `OpError.Error()` formatting, not a documented contract - it's already two different strings depending on exactly how the underlying read failed, and any future dependency bump could change it again and silently start logging a message on every single timeout tick. ## Plan `mdlayher/netlink.OpError` (the actual concrete error type `Conn.Receive()` returns) already implements the standard `net.Error` interface, including a correct `Timeout() bool` that unwraps through `*os.SyscallError` where needed. Replace the string comparison with the idiomatic, documented Go approach: ```go var netErr net.Error if errors.As(err, &netErr) && netErr.Timeout() { // Routine read-timeout wakeup - not a real error, ignore. return 0 } ``` This is robust to message-text changes across library versions (it 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 (`errors.As` walks the full `Unwrap()` chain). The existing test (`TestNfErrorFunction`) currently constructs plain `errors.New("netlink receive: i/o timeout")` strings, which only exercises the *current* buggy approach faithfully - it'll be updated to construct a real `*netlink.OpError` wrapping a real timeout-capable error (proving detection works against the actual type this function receives in production, not just a string that happens to match today), plus a case wrapped an extra level deeper (to prove the new check's advantage over string matching), and a plain non-timeout error (unchanged "still logs" case). No behavior change for real production traffic - this is purely a correctness/robustness fix for how "is this a benign read-timeout" is detected. Work is happening on branch `issue16_error_handling`; an MR will follow referencing this issue.

danny commented

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

(Migrated from gitlab.keyop.co.uk)

mentioned in commit 8e9e183a52

mentioned in commit 8e9e183a52e7da6289097fafd5109b589e16186a

danny commented

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

(Migrated from gitlab.keyop.co.uk)

mentioned in merge request !65

danny commented

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

(Migrated from gitlab.keyop.co.uk)

mentioned in commit 50948e433b

mentioned in commit 50948e433b251d459df637234835b1685c543485

danny (Migrated from gitlab.keyop.co.uk) closed this issue

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

danny commented

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

(Migrated from gitlab.keyop.co.uk)

Fixed via MR !65 and released in v1.0.4 (https://gitlab.keyop.co.uk/keyop/go/nfq_forwarder/-/releases/v1.0.4). nfErrorFunction now detects a routine netlink read timeout via the standard net.Error/Timeout() interface (errors.As) instead of matching hardcoded error-message strings. Closing.

Fixed via MR !65 and released in **v1.0.4** (https://gitlab.keyop.co.uk/keyop/go/nfq_forwarder/-/releases/v1.0.4). `nfErrorFunction` now detects a routine netlink read timeout via the standard `net.Error`/`Timeout()` interface (`errors.As`) instead of matching hardcoded error-message strings. Closing.

Rows
Columns

nfErrorFunction does string matching to determine error type #16

Still valid - and it matters more than it looks

Plan

`nfErrorFunction` does string matching to determine error type #16