Skip to content

Remove DRPC per-function request queues once they are empty - #8985

Merged
rzo1 merged 1 commit into
masterfrom
fix/drpc-function-queue-eviction
Aug 22, 2026
Merged

Remove DRPC per-function request queues once they are empty#8985
rzo1 merged 1 commit into
masterfrom
fix/drpc-function-queue-eviction

Conversation

@rzo1

@rzo1 rzo1 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

The map of function name to request queue in DRPC gained an entry for each function name a client asked about, and no code path ever removed one: cleanup() removed the request from a queue and cleanupAll() drained queues, but the queue object and its map entry stayed for the life of the process.

Entries are now dropped once nothing is waiting in them, using compute/computeIfPresent so the map stays race-free. A registered function is served exactly as before, its queue simply being recreated by the next execute(). Extends DRPCTest.

@rzo1 rzo1 added this to the 3.1.0 milestone Aug 21, 2026
@rzo1 rzo1 self-assigned this Aug 21, 2026
@reiabreu

Copy link
Copy Markdown
Contributor

Disclaimer: this comment was generated with the help of an LLM.

The new test proves the map returns to empty (the leak fix), but it's essentially sequential — it wouldn't catch a lost-request race. Since correctness here depends on every queue mutation going through the per-key compute lock, a concurrent stress test would strengthen it. Rough sketch:

@Test
public void concurrentSubmitAndFetchLosesNoRequests() throws Exception {
    try (DRPC server = new DRPC(new StormMetricsRegistry(), null, 60_000)) {
        int n = 10_000;
        for (int i = 0; i < n; i++) {
            exec.submit(() -> server.executeBlocking("fn", "x"));
        }
        int served = 0;
        while (served < n) {
            DRPCRequest req = server.fetchRequest("fn");
            if (req != null && !req.get_request_id().isEmpty()) {
                server.returnResult(req.get_request_id(), "ok");
                served++;
            }
        }
        assertEquals(n, served);                          // none lost / duplicated
        assertEquals(0, server.getNumTrackedFunctions()); // no queue left behind
    }
}

Not a blocker — just closes the gap on the part of the change that's hardest to get right.

@rzo1
rzo1 force-pushed the fix/drpc-function-queue-eviction branch from 12f324a to 962a595 Compare August 22, 2026 16:54
@rzo1
rzo1 force-pushed the fix/drpc-function-queue-eviction branch from 962a595 to 65c36aa Compare August 22, 2026 18:44
@rzo1

rzo1 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Disclaimer: this comment was generated with the help of an LLM.

The new test proves the map returns to empty (the leak fix), but it's essentially sequential — it wouldn't catch a lost-request race. Since correctness here depends on every queue mutation going through the per-key compute lock, a concurrent stress test would strengthen it. Rough sketch:

@Test
public void concurrentSubmitAndFetchLosesNoRequests() throws Exception {
    try (DRPC server = new DRPC(new StormMetricsRegistry(), null, 60_000)) {
        int n = 10_000;
        for (int i = 0; i < n; i++) {
            exec.submit(() -> server.executeBlocking("fn", "x"));
        }
        int served = 0;
        while (served < n) {
            DRPCRequest req = server.fetchRequest("fn");
            if (req != null && !req.get_request_id().isEmpty()) {
                server.returnResult(req.get_request_id(), "ok");
                served++;
            }
        }
        assertEquals(n, served);                          // none lost / duplicated
        assertEquals(0, server.getNumTrackedFunctions()); // no queue left behind
    }
}

Not a blocker — just closes the gap on the part of the change that's hardest to get right.

Added something in that way. Thanks

@rzo1
rzo1 merged commit 4af262d into master Aug 22, 2026
2 checks passed
@rzo1
rzo1 deleted the fix/drpc-function-queue-eviction branch August 22, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants