Skip to content

fix: preserve hooks for added writer interfaces - #35

Merged
felixge merged 2 commits into
masterfrom
fix-preserve-hooks-pr33
Jun 11, 2026
Merged

fix: preserve hooks for added writer interfaces#35
felixge merged 2 commits into
masterfrom
fix-preserve-hooks-pr33

Conversation

@felixge

@felixgefelixge commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Follow-up to #33.

#33 added support for io.StringWriter and FlushError, but those new interfaces changed method dispatch in ways that could bypass existing hooks:

  • io.WriteString preferred WriteString, bypassing an existing Hooks.Write interceptor when no Hooks.WriteString was configured.
  • http.ResponseController.Flush preferred FlushError, bypassing an existing Hooks.Flush interceptor when no Hooks.FlushError was configured.

This preserves the previous hook behavior by routing the new interface methods through the older hook when only the older hook is configured. For the FlushError fallback, the old Flush hook wraps the underlying FlushError call so the returned error is preserved.

Hook behavior after this change

Wrap(w, hooks) still only exposes optional interfaces that w already exposes. Hooks do not add new capabilities.

General rule:

  • For each method, the exact matching hook wins.
  • If no exact hook is configured, most methods call the underlying method directly.
  • The hook chain is prepared once during Wrap, not rebuilt on every method call.

Normal http.ResponseWriter methods:

CallIf hook is configuredOtherwise
Header()Hooks.Headerw.Header()
WriteHeader(code)Hooks.WriteHeaderw.WriteHeader(code)
Write(p)Hooks.Writew.Write(p)

Optional methods follow the same exact-hook rule for Flush, FlushError, CloseNotify, Hijack, ReadFrom, SetReadDeadline, SetWriteDeadline, EnableFullDuplex, Push, and WriteString.

Compatibility fallback for WriteString when the underlying writer implements io.StringWriter:

Hooks configuredWriteString("x") behavior
WriteStringCalls Hooks.WriteString
only WriteCalls Hooks.Write with []byte("x")
both Write and WriteStringWriteString wins; Write is not called automatically
neitherCalls underlying w.WriteString("x")

If the underlying writer does not implement io.StringWriter, the wrapper does not either, so io.WriteString(wrapped, s) falls back to Write([]byte(s)) and Hooks.Write applies as before.

Compatibility fallback for FlushError when the underlying writer implements both http.Flusher and FlushError:

Hooks configuredFlushError() / ResponseController.Flush() behavior
FlushErrorCalls Hooks.FlushError
only FlushCalls Hooks.Flush, whose next calls underlying FlushError; returns the underlying FlushError error
both Flush and FlushErrorFlushError wins; Flush is not called automatically
neitherCalls underlying FlushError()

If the underlying writer only implements http.Flusher, ResponseController.Flush() uses Flush(), so Hooks.Flush applies. If it only implements FlushError, Hooks.Flush cannot apply because there is no Flush() method to wrap; use Hooks.FlushError.

Tests added for both regressions, including preserving the underlying FlushError error through the Flush compatibility fallback.

Validation:

go test ./...

@felixge
felixgeforce-pushed the fix-preserve-hooks-pr33 branch from 7d17cb2 to 1c1fc4cCompareJune 11, 2026 12:05
@felixge
felixge marked this pull request as ready for review June 11, 2026 12:05
Comment threadwrap_test.go
Comment threadcodegen/main.go Outdated
// http.ResponseController.Flush prefers FlushError over Flush.
// Preserve existing Flush hooks when wrapping writers that expose both.
g.Printf("} else if state.flush != nil {\n")
g.Printf("state.flushError = func() error { state.flush(); return nil }\n")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should use the old hook behaviour in this case so we can actually return the error.

Something like:

state.flushError=func() (errerror) {
hooks.Flush(func() { err=t1.FlushError()
})()
returnerr
}

@felixge
felixge merged commit 0fc9006 into masterJun 11, 2026
4 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@felixge@boekkooi-impossiblecloud