Hi @atzoum @mihir20,
While looking at the webhook batch processing path (in the context of #6787, fixed via #6835 / in progress in #6988), I noticed
there is an unrelated TODO in the same file that is still outstanding and looks small/self-contained enough for a follow-up
chore.
Reference
gateway/webhook/webhook.go:320
// TODO : return back immediately for blank request body. its waiting till timeout
This TODO predates the context/timeout work and was not addressed by #6835 or #6988.
Current behavior
In HandleT.RequestHandler (gateway/webhook/webhook.go:127), after form / multipart parsing (lines 141–208) we always enqueue the
request:
requestQ <- &req // webhook.go:229
// ...
resp := <-done // webhook.go:233
If the incoming request has a blank body and no form/multipart payload (so jsonByte is empty and the original body is empty too),
the request still:
1. Gets pushed onto the per-source requestQ.
2. Sits in batchRequests (webhook.go:287) until either:
- the buffer reaches maxWebhookBatchSize (default 32, setup.go:63), or
- the webhookBatchTimeout ticker fires (default 20ms, setup.go:65).
3. Is then handed to batchTransformLoop, where the source transformer eventually rejects it.
The end result is the caller waits at least a full batch interval (and burns a transformer call) for a request we can already
tell is invalid at the HTTP layer.
Proposed fix
Reject blank-body requests in RequestHandler before they hit requestQ, after the existing form / multipart parsing has had a
chance to populate jsonByte. Rough shape:
// Reject blank request bodies immediately instead of waiting for the batch
// timeout / source transformer round-trip. GET-allowed sources
// (forwardGetRequestForSrcMap) carry their payload in the URL and are exempt.
if r.Method != http.MethodGet && len(jsonByte) == 0 && r.ContentLength == 0 {
stat := webhook.statReporterCreator(arctx, reqType)
stat.RequestFailed(response.NoRequestBody)
stat.Report(webhook.stats)
webhook.failRequest(w, r,
response.GetStatus(response.NoRequestBody),
response.GetErrorStatusCode(response.NoRequestBody))
webhook.ackCount.Add(1)
return
}
Supporting changes:
- Add a NoRequestBody error constant in gateway/response (HTTP 400), following the pattern of the existing error constants in
that package.
- Add a unit test in gateway/webhook/webhook_test.go asserting that a POST webhook with an empty body returns 400 quickly,
without waiting for webhookBatchTimeout to elapse.
- Remove the now-resolved TODO comment at webhook.go:320.
Out of scope
- The other TODO at webhook.go:324 ("Remove timeout from here after timeout handler is added in gateway") — already being
addressed in #6787 / #6988.
- Any change to the batch sizing or batch timeout defaults.
- Behavior of GET-allowed sources (those listed in Gateway.webhook.forwardGetRequestForSrcs) — they remain unchanged because they
can legitimately carry data in the URL.
Hi @atzoum @mihir20,
While looking at the webhook batch processing path (in the context of #6787, fixed via #6835 / in progress in #6988), I noticed
there is an unrelated TODO in the same file that is still outstanding and looks small/self-contained enough for a follow-up
chore.
Reference
gateway/webhook/webhook.go:320