collections: LinearFifo drops its live items and iterates both ring halves

Closed
AL
3mo ago
+304-9

The problem

LinearFifo stores its items in MaybeUninit slots. Rust does not drop MaybeUninit contents. The comment in linear_fifo.rs said that field drop glue covers deinit. That is only true for types with no drop glue. If a fifo of Box or of JS promise handles is dropped with items still queued, the storage is freed but the items are not dropped. They leak.

Both RedisClient queues hold such items. The offline queue holds Entry (a Box<[u8]> and a promise Strong). The in flight queue holds PromisePair (a promise Strong). Today shutdown() and reject_all_pending_commands() drain both queues by hand with read_item() before the queues are dropped, so I did not find a live leak in the Redis client. The leak is one missed drain away, and any new fifo of a droppable type gets it for free. discard() and ordered_remove_item() had the same gap. They moved head past the items or memmoved over them without a drop.

readable_slice(0) returns only the first contiguous run of the ring. Once the ring wraps, the second run is not in the slice. Two RedisClient sites used it as a whole queue scan. The auto pipeline flush counted pipelineable commands from it, so a wrapped queue was flushed in two socket writes instead of one. The memoryCost estimate summed serialized bytes from it, so it under reported a wrapped queue. Both were found while reviewing #34829 and #39193.

What changed

LinearFifo now implements Drop. It drops the live items in ring order, both runs. discard(n) drops the first n items before it moves head. read_item() moves the item out and then advances head without a drop, so nothing is dropped twice. ordered_remove_item() drops the removed item before the shift. Types without drop glue take the same fast path as before.

New accessors: as_slices(), as_mut_slices(), iter() and iter_mut(). They cover all live items across both runs in FIFO order. The two scan sites in valkey.rs and js_valkey.rs now use iter(). The head peeks at send_next_command() and drain() only read element 0 of readable_slice(0). That element is always in the first run, so they are unchanged.

Tests

Unit tests in linear_fifo.rs use a type that counts its drops and owns a Box. They cover drop of a partially read fifo, drop of a wrapped ring, drop of a static ring, discard across the wrap, read_item not dropping, ordered_remove_item on both runs, iter() and iter_mut() on empty, full, wrapped and grown rings, and iter() matching repeated read_item(). The three drop tests fail on main.

Not in this PR

The MaybeUninit accessor rework noted at the top of the file. writable_slice() still hands out &mut [T] over unwritten slots. That needs a signature change in other crates.