Fix a streaming bug + document streaming through example#1765
Conversation
…allowed, making it hard to track down
|
I might need a bit of help devising a test-strategy for this. |
the server side will stagger writes and the client side will verify that this is happening. buffering will make the tests fail.
|
Sorry about all the noisy commits. It took me long time to figure out how to align the go.mod files properly. |
|
thanks for this PR. Hopefully it gets merged. The update to the SSE endpoint logic fixed SSE for us! |
|
Hi @jamietanna, Can we get this reviewed/merged? It is blocking us from using it on our next AI project. Thanks!! |
|
Note that this PR is a lot bigger than it needed to be because @jamietanna also asked me to document this, which dramatically expanded the scope of the fix. It's been a long time since I did this, but I think the actual bug fix is just the code in pkg/codegen/templates/strict/strict-interface.tmpl, not all the rest. |
|
Something to consider, the behavior changes per target server. the Standard http server works properly with the current implementation, while gin requires the flush for it to stream. |
This doesn't seem to be the case. I've just ran into this again, with the stdlib server. |
|
@jamietanna Hi, anything we can do to help get this merged? The current workarounds for this are a bit awkward. Thanks! |
Greptile SummaryThis PR fixes SSE/streaming buffering in the strict-server code generators by replacing
Confidence Score: 4/5Safe to merge for net/http, Chi, Gorilla, and Iris users; Fiber streaming remains unfixed despite appearing to be handled. The core fix for net/http-based strict servers is correct and well-structured. However, the Fiber template's http.Flusher assertion will always fail at runtime (fasthttp types don't implement net/http interfaces), meaning Fiber users get no benefit from the streaming fix and may be unaware of this. This is a present functional defect on the Fiber code path. pkg/codegen/templates/strict/strict-fiber-interface.tmpl needs a Fiber-native flushing approach instead of http.Flusher.
|
| Filename | Overview |
|---|---|
| pkg/codegen/templates/strict/strict-fiber-interface.tmpl | Uses http.Flusher type assertion on Fiber's fasthttp writer, which always fails — streaming optimisation is silently skipped for all Fiber users. |
| pkg/codegen/templates/strict/strict-interface.tmpl | Adds flush-per-chunk streaming loop for net/http-based strict servers when content-type is a configured streaming type; fallback to io.Copy is correct. |
| pkg/codegen/templates/strict/strict-iris-interface.tmpl | Applies http.Flusher-based flush loop for Iris; Iris wraps net/http so the assertion is expected to succeed at runtime. |
| pkg/codegen/configuration.go | Adds StreamingContentTypes to OutputOptions with validation (compileStreamingContentTypes) that returns errors for invalid regexes; defaults are text/event-stream, application/jsonl, application/x-ndjson. |
| examples/streaming/stdhttp/sse/impl.go | Working SSE implementation using io.Pipe for async writes; ContentLength: 0 relies on zero-value meaning "unknown" (chunked), which is consistent with streaming semantics. |
| examples/streaming/stdhttp/main.go | Entry point for the streaming example server; contains a spurious trailing colon in a slog key ("error:" instead of "error"). |
Reviews (2): Last reviewed commit: "streaming support in fiber/iris" | Re-trigger Greptile
Revert the PR's additions to internal/test/strict-server so the existing harness is left intact. The streaming template change and dedicated example under examples/streaming/ remain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # pkg/codegen/templates/strict/strict-interface.tmpl
Make an end-to-end example, with a client that prints the stream from the server. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@perbu , I've modified your code. I removed the strict-server changes and extended your example with a client which connects to the server, and prints out its event stream. However, I don't think we're done here. What you call Both of these two streaming formats need different implementations. |
The strict-server template previously hard-coded a single content-type
check (text/event-stream) to decide when to generate a flush-per-chunk
response path. Replace the string match with a configurable, regex-based
mechanism so the decision is declared once and applies to any streaming
media type.
- Add OutputOptions.StreamingContentTypes (list of regex patterns) and
a defaultStreamingContentTypes list of text/event-stream,
application/jsonl, and application/x-ndjson. User-provided patterns
are merged on top of the defaults.
- Compile the merged list up-front: OutputOptions.Validate() rejects
invalid regexes via the existing CLI validation gate, and Generate()
stashes the compiled regexes in globalState for template access.
- Expose ResponseContentDefinition.IsStreamingContentType(), mirroring
the existing IsJSON/IsSupported helpers, so strict-interface.tmpl can
use {{if .IsStreamingContentType}} instead of an inline string match.
The generated streaming code itself is unchanged; only the gate moved.
- Document the new option in configuration-schema.json.
- Update the streaming example spec from text/event-stream to
application/jsonl, which matches what the server actually emits
(newline-delimited JSON). The impl.go response-type reference is
renamed to track the regenerated type name. application/jsonl is in
the default list, so the streaming path still activates.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Ok, I've made another change now, where the streaming logic that copies chunks isn't based on a hard coded MIME type, but rather a template helper function which determines if the streaming code needs to be emitted. This is configured on the Configuration, which some reasonable defaults. I renamed your example's functions to be consistent with their jsonlines behavior. |
|
hi @mromaszewicz. Thanks for picking this up! I've been going back to this PR even though I've moved on to entirely new tasks, but I still, from time to time, need to make oapi-codegen-based servers that push SSE. Let me know if there is anything I can do. |
Port the flush-per-chunk response path from the stdhttp strict template into the fiber and iris variants, gated on the same IsStreamingContentType helper that was just added. Non-streaming responses still use the existing io.Copy call unchanged. - Iris uses the same http.Flusher type-assertion pattern as stdhttp, since iris.ResponseWriter implementations satisfy the interface. Falls back to io.Copy when the assertion fails. - Fiber uses fasthttp's SetBodyStreamWriter callback, which delivers a chunk to the client on each w.Flush() call — the body close moves inside the callback because fasthttp invokes it asynchronously. goimports adds the required bufio import when the streaming branch is actually emitted. The broader drift between the three strict-interface templates is tracked separately in oapi-codegen#2331 and is not touched here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The server example used "error:" as a slog key, which slog treats as a literal attribute name instead of the intended "error" key. Surfaced by Greptile review on PR oapi-codegen#1765. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 participants
This PR fixes two issues related to streaming of server-sent events (SSE). One is documenting streaming responses through an example. This showcases how an SSE endpoint could be specified and implemented in strict mode.
The second issue is a bug. Currently the generated code uses io.Copy() to copy to the response socket. This will allow the underlying OS to buffer the response for an arbitrary amount of time. A text/event-stream is typically time-sensitive so a flush of the socket should be forced after each write.
The PR will change the templated code and if the content-type is text/event-stream the io.Copy() will be replaced with a for-loop copying and flushing the data after each write.
Fixes: #1764