out_azure_kusto: Add changes to defer close old resource handles in subsequent cycles#11418
out_azure_kusto: Add changes to defer close old resource handles in subsequent cycles#11418ag-ramachandran wants to merge 4 commits intofluent:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds three deferred-cleanup fields and implements a two-stage resource rotation in the Azure Kusto plugin to delay destruction of previous upstreams and identity tokens while new resources are installed, preventing use-after-free during high-volume concurrent operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as azure_kusto_load_ingestion_resources
participant Parser as storage_parser
participant Identity as IdentityService
participant Active as ActiveResources
participant Old as OldResources
Loader->>Parser: parse storage config
Parser-->>Loader: storage parameters
Loader->>Identity: request new identity token
Identity-->>Loader: return new token
Loader->>Old: destroy existing old_blob_ha/old_queue_ha/old_identity_token (if any)
Loader->>Old: move Active.blob_ha -> Old.old_blob_ha\nActive.queue_ha -> Old.old_queue_ha\nActive.identity_token -> Old.old_identity_token
Loader->>Active: assign new blob_ha, queue_ha, identity_token
Loader->>Loader: update load_time
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The patch looks good but we need to add a |
33d6e3b to
9f8e03e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/out_azure_kusto/azure_kusto_conf.c`:
- Around line 616-638: The rotation logic in
azure_kusto_load_ingestion_resources moves and destroys HA and token objects
while worker functions azure_kusto_create_blob and azure_kusto_enqueue_ingestion
may access them without holding resources_mutex, leading to use-after-free; fix
by ensuring callers grab stable references under the mutex (or an equivalent
refcount) before using them: in azure_kusto_create_blob and
azure_kusto_enqueue_ingestion acquire resources_mutex, copy
ctx->resources->blob_ha/queue_ha and ctx->resources->identity_token into local
variables, call flb_upstream_ha_node_get (or increment the object's refcount)
while still holding the lock, then release the lock and use the obtained
node/token, and update azure_kusto_load_ingestion_resources to only destroy
old_* objects after they are no longer referenced (i.e., after refcount reaches
zero) or ensure destruction happens under the same mutex that prevents new gets.
Signed-off-by: ag-ramachandran <[email protected]>
9f8e03e to
f270724
Compare
Signed-off-by: ag-ramachandran <[email protected]>
Hello @cosmo0920, Fixed the comments. Tested as well and so far it looks okay. If it looks okay, kindly review/approve the same. |
This pull request improves the safety and stability of resource management in the Azure Kusto output plugin by introducing deferred destruction of old resources. This change helps prevent use-after-free errors during high-volume operations where resources might still be in use by other threads when a refresh occurs.
Resource lifecycle management improvements:
old_blob_ha,old_queue_ha,old_identity_token) tostruct flb_azure_kusto_resourcesto track old resources that are pending cleanup, enabling deferred destruction.azure_kusto_load_ingestion_resourcesto first destroy resources from two refresh cycles ago, then move current resources to the "old" fields before assigning new ones, ensuring safe resource transitions.flb_azure_kusto_resources_clearto clean up any old resources pending destruction, ensuring proper resource deallocation and preventing memory leaks.Summary by CodeRabbit