zeroshade opened a new pull request, #801:
URL: https://github.com/apache/arrow-go/pull/801
### Rationale for this change
Fixes #755. The cookie middleware (`NewClientCookieMiddleware`) does not
capture `Set-Cookie` headers returned in response to a streaming RPC like
`Handshake` when the server also sends back a response payload.
`ClientHeadersMiddleware.HeadersReceived` was only invoked from `finishFn`,
which fires when `Recv()` returns `io.EOF` or a non-`io.EOF` error.
`AuthenticateBasicToken` calls `Recv()` exactly once; if the server sends a
`HandshakeResponse` payload (common when the Handshake carries auth data or a
session cookie), `Recv()` returns that message rather than `io.EOF` and
`finishFn` never fires. The cookie middleware never sees the response headers,
so the session cookie is dropped and subsequent RPCs go out without it, even
though the user reports cookies ARE delivered on other endpoints like
`GetFlightInfo` (unary RPCs capture headers synchronously via
`grpc.Header(&md)`).
### What changes are included in this PR?
- `clientStream.Header()` now delivers response metadata to
`ClientHeadersMiddleware` at-most-once (guarded by
`atomic.Bool.CompareAndSwap`) the first time headers are successfully retrieved
for a streaming RPC.
- The existing `finishFn` path is unchanged so:
- trailers are still captured when the stream completes, and
- callers that never explicitly invoke `Header()` get the exact same
behavior as before.
- Added four regression tests in `arrow/flight/handshake_cookie_test.go`
covering:
1. `Set-Cookie` in Handshake response **headers** (via
`AuthenticateBasicToken`)
2. `Set-Cookie` in Handshake response **trailers**
3. `Set-Cookie` + server-sent `HandshakeResponse` payload (the precise
scenario reported in #755 — fails without this fix)
4. Eager capture when `stream.Header()` is inspected before draining the
stream (also fails without this fix)
### Are these changes tested?
Yes. The four new tests in `arrow/flight/handshake_cookie_test.go` reproduce
the regression. Tests 3 and 4 fail without the fix and pass with it. The
existing middleware/cookie tests continue to pass, including with `-race`.
### Are there any user-facing changes?
Minor behavioral refinement of `ClientHeadersMiddleware` for streaming RPCs:
`HeadersReceived` may now be invoked up to twice on a streaming RPC whose
caller explicitly calls `stream.Header()` — once with just the response headers
(from `Header()`), and again with headers+trailers joined (from the existing
`finishFn` path at stream completion). This is backward compatible for the
in-tree `clientCookieMiddleware` (cookie updates are keyed by `name+path` and
idempotent). Callers that never explicitly call `stream.Header()` see no change
in behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
