Normalized send memory allocation: added user context associated with… #20
Conversation
WalkthroughAdds a per-send void* user_context to stream sends: updates wtf_stream_send signature, extends SEND_COMPLETE event payload to include user_context, propagates the context through the internal send context and types, adjusts buffer lifetime handling, and updates call sites (e.g., wtf_stream_close). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Stream as wtf_stream_send
participant SendCtx as send_context
participant Transport as QUIC_Send
participant App as EventCallback
Caller->>Stream: wtf_stream_send(buffers, count, fin, user_context)
Stream->>SendCtx: allocate send_ctx, copy buffers, store user_context
Stream->>Transport: submit send (send_ctx->quic_buffers, ClientContext=send_ctx)
Transport-->>Stream: send completion
Stream-->>App: SEND_COMPLETE{buffers, count, cancelled, user_context}
Stream->>SendCtx: cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
include/wtf.h (3)
263-264: Two user_context fields in wtf_stream_event_t may confuse API consumersTop-level event->user_context is the stream-level context (wtf_stream_set_context), and send_complete.user_context is the per-send context (wtf_stream_send). Consider clarifying naming in docs and examples to avoid misuse.
Apply this minimal doc tweak to reduce ambiguity:
- void* user_context; //! User-provided context data (wtf_stream_set_context) + void* user_context; //! Stream-level user context (set via wtf_stream_set_context)
560-561: New user_context parameter docs are clearThe parameter description reads well and conveys the behavior. Consider also stating that the library doesn't own/free this pointer and that its lifetime must extend through the send completion.
-//! @param user_context an opaque context to be passed back to the caller on completion +//! @param user_context an opaque context to be passed back to the caller on completion. +//! The library does not take ownership and will not free this pointer. It must remain +//! valid until the WTF_STREAM_EVENT_SEND_COMPLETE callback fires.
577-579: Doc note is misleading given buffer array lifetime managementThe current note says "passes the original buffers directly to the QUIC layer without modification or copying." With the new internal array to fix ownership over the buffer array, we should clarify that no data copying occurs, but the array headers may be shallow-copied.
-//! @note The function passes the original buffers directly to the QUIC layer without -//! modification or copying. No protocol headers are added for stream data. +//! @note No protocol headers are added for stream data. The buffer data pointers are not copied. +//! The library may shallow-copy the buffer descriptors to manage their lifetime across +//! the asynchronous send completion.src/stream.c (1)
172-183: Initialize user_context for internal (header) sends to avoid accidental useFor internal header sends, user_context is currently uninitialized. While it's not read for internal sends today, future changes could inadvertently access it. Initialize to NULL.
send_ctx->buffers = header_buffer; send_ctx->count = 1; + send_ctx->user_context = NULL; send_ctx->internal_send = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/wtf.h(4 hunks)src/stream.c(5 hunks)src/types.h(1 hunks)
🔇 Additional comments (5)
src/types.h (1)
325-328: Per-send user_context addition is fine; make sure all internal send contexts initialize itAdding the per-send user_context to wtf_internal_send_context is aligned with the PR objective. Ensure every allocation path for this struct explicitly initializes user_context (NULL for internal sends) to avoid accidental use of uninitialized memory in future changes.
include/wtf.h (2)
273-277: SEND_COMPLETE payload enriched with per-send context — goodExposing the per-send user_context in the send_complete payload matches the API extension and unblocks apps from correlating completions. No issues found.
580-581: API break: wtf_stream_send signature changeNo internal uses of
wtf_stream_sendwere found in.c,.cc, or.cppfiles in this repo. This update still introduces a source-breaking change for external consumers:
- Location:
include/wtf.hlines 580–581 (now takes an extravoid* user_contextparameter)- Ensure all consumer call sites are updated to pass the new
user_contextargument- Consider adding a temporary compatibility shim for the 4-argument overload
- Document this change in release notes and bump the library’s version
src/stream.c (2)
273-289: Per-send user_context is correctly propagated to SEND_COMPLETEThe event now includes send_ctx->user_context, enabling callers to correlate completions without relying on external state. This matches the API update.
668-669: wtf_stream_close updated to pass NULL per-send context — goodPreserves previous semantics while conforming to the new API. Looks good.
… sent buffers; fixed ownership over buffer array
roman380
force-pushed
the
SendMemoryOwnership
branch
from
8c6ec13 to
a242fe0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
include/wtf.h (3)
259-276: Disambiguate per-stream vs per-send context fields in wtf_stream_event_tThere are two user_context pointers in this struct: one at the top-level (stream-scoped) and one in send_complete (per-send). Rename the latter to avoid ambiguity in client code.
typedef struct { wtf_stream_event_type_t type; //! Type of stream event wtf_stream_t* stream; //! Stream that generated the event - void* user_context; //! User-provided context data (wtf_stream_set_context) + void* user_context; //! Stream-scoped context (wtf_stream_set_context) union { struct { wtf_buffer_t* buffers; //! Array of received data buffers uint32_t buffer_count; //! Number of buffers bool fin; //! True if this is the final data } data_received; struct { wtf_buffer_t* buffers; //! Array of sent data buffers uint32_t buffer_count; //! Number of buffers sent - void* user_context; //! User-provided context data (wtf_stream_send) + void* send_user_context; //! Per-send context (from wtf_stream_send) bool cancelled; //! True if send was cancelled } send_complete;
563-579: Clarify lifetime of buffer descriptors in wtf_stream_send docsThe stream send implementation allocates and copies only the array of
wtf_buffer_tdescriptors (not the buffer data) into an internal array, then frees that internal array on send completion. Update the docs to reflect that callers may free their ownbuffersarray immediately, but must keep eachbuffers[i].datavalid until theWTF_STREAM_EVENT_SEND_COMPLETEevent.• Location:
include/wtf.h, aroundwtf_stream_senddoc (lines ~563–579)
• Replace the ambiguous note with:- //! @note The function passes the original buffers directly to the QUIC layer without - //! modification or copying. No protocol headers are added for stream data. + //! @note The function copies your array of buffer descriptors (pointers and lengths) + //! into an internal array but does not copy the actual data. You may free the + //! `buffers` array itself immediately after calling `wtf_stream_send`. However, + //! each `buffers[i].data` pointer must remain valid until the library emits + //! `WTF_STREAM_EVENT_SEND_COMPLETE`, at which point the internal descriptors + //! are freed. No protocol headers are added for stream data.
555-581: Preserve public API compatibility: introduce wtf_stream_send_ex and retain an inline wtf_stream_send wrapperThe new five-parameter signature for
wtf_stream_sendis an ABI break. To maintain the existing four-parameter API, rename the implementation towtf_stream_send_ex, provide an inline wrapper forwtf_stream_sendthat passesNULLforuser_context, and update the docs accordingly.Files to update:
- include/wtf.h (around line 579): rename the API entry, insert the
_exvariant and inline wrapper, and adjust the@paramcomments.- src/stream.c (around line 568): rename the definition from
wtf_stream_sendtowtf_stream_send_ex.- samples/echo_server.c: existing calls to
wtf_stream_send(...)will continue to work via the inline wrapper.- README.md (examples at lines 92 and 118): ensure calls omit
user_contextor add a note about the new_exvariant.Example diff:
//! @param fin true if this is the final data -//! @param user_context an opaque context to be passed back to the caller on completion +//! @param user_context an opaque context to be passed back to the caller on completion (…_ex only) //! @return WTF_SUCCESS on success, error code on failure … -WTF_API wtf_result_t wtf_stream_send(wtf_stream_t* stream, const wtf_buffer_t* buffers, - uint32_t buffer_count, bool fin, void* user_context); +// Extended variant with per-send context. +WTF_API wtf_result_t wtf_stream_send_ex(wtf_stream_t* stream, const wtf_buffer_t* buffers, + uint32_t buffer_count, bool fin, void* user_context); +// Backward-compatible inline wrapper preserving the original signature. +static inline wtf_result_t wtf_stream_send(wtf_stream_t* stream, const wtf_buffer_t* buffers, + uint32_t buffer_count, bool fin) { + return wtf_stream_send_ex(stream, buffers, buffer_count, fin, NULL); +}Please confirm whether maintaining source compatibility is required for this release; if not, document this breaking change in the release notes and bump the major version.
🧹 Nitpick comments (1)
src/types.h (1)
321-328: Const-correctness and ownership note for internal send contextGood addition of per-send user_context. Two improvements:
- Make buffers const to prevent accidental mutation of caller data.
- Document that user_context is opaque and not owned/freed by the library.
Apply:
typedef struct { - wtf_buffer_t* buffers; + const wtf_buffer_t* buffers; uint32_t count; - void* user_context; + void* user_context; // opaque; lifetime managed by the caller; not owned/freed by the library wtf_session* session; bool internal_send; } wtf_internal_send_context;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/wtf.h(4 hunks)src/stream.c(5 hunks)src/types.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stream.c
|
I’m going to hold off merging this for the moment as I have upstream patches I need to make to msquic |
|
No problem, I am sure these changes will survive upstream merge and send part is perfect now. We are likely to extend similarly to datagrams when it comes to audio. I'd say the bigger problem is #21 now, but we'll also have some time to find a solution as context/server instance bandaided as a singleton without destruction seems to be sufficient to make acceptable experience integrating this further. |
|
Does this fix the double |
Hard to tell, we had to put this on pause a bit and so I just don't remember already. We did have this working pretty well as a prototype though. |
|
I think if line 547 in I pulled this code and tested. Can't tell the difference between this PR and main after commenting that |
3 participants
… sent buffers; fixed ownership over buffer array
Summary by CodeRabbit
New Features
Documentation