Repository navigation
Fix: if calls Close or Dispose many times it'll throws a NullReferenceException - #3
Merged
Merged
Conversation
mgravell
added a commit
that referenced
this pull request
Mar 22, 2014
Fix: if calls Close or Dispose many times it'll throws a NullReferenceException
Collaborator
|
Thanks |
mgravell
added a commit
that referenced
this pull request
Aug 25, 2026
Bits/Index both said the wrong thing: Index named the mechanism ("an index")
when the question is *of what* - bits or elements - and Bits(100) read as a
quantity ("100 bits") rather than a position ("bit 100").
Singular noun plus ordinal, symmetrically: Bit(600) is bit 600, Element(3) is
element 3, i.e. the #3 form the server multiplies by the width. Element rather
than Field because "field" is already carrying weight in BitFieldEncoding and
BitFieldOperation, and would read as naming a field rather than counting one.
mgravell
added a commit
that referenced
this pull request
Aug 25, 2026
…TPOS (#3196) * Fill in the neglected bitmap commands: BITFIELD, and an open-ended BITPOS Two gaps in the bitmap family (https://redis.io/docs/latest/commands/?group=bitmap). BITFIELD and BITFIELD_RO were entirely unimplemented - the only trace in the repo was a commented-out line in the envoy exclusion list. Added as: long? StringBitField(key, BitFieldOperation, flags) Lease<long?>? StringBitField(key, ReadOnlyMemory<BitFieldOperation>, flags) plus BitFieldEncoding (Signed/Unsigned, and named Int8..Int64/UInt8..UInt63), BitFieldOffset (Bits/Index, the latter being the server's #N form), BitFieldOverflow (Wrap/Saturate/Fail) and BitFieldOperation (Get/Set/ IncrementBy). Signedness lives with the width rather than in a flags enum, and #N lives on the offset, so no flag silently reinterprets another argument. Every legal encoding (i1-i64, u1-u63) fits losslessly in a signed 64-bit integer, which is exactly why u64 does not exist, so the results are plain long - nullable only because OVERFLOW FAIL reports a nil for the operation it skipped. There is deliberately a named UInt63 where UInt64 would sit, to document the ceiling at the point someone reaches for it. The OVERFLOW token is sticky server-side and untouched by GET, so it is emitted only when the required mode changes, and never for a leading run of WRAP (the server default). An all-GET batch is issued as BITFIELD_RO where the server supports it (6.0+) so a replica can serve it; anything that writes stays as BITFIELD, which is primary-only. An empty batch short-circuits locally. Retry category follows the payload: accumulating by default, demoted to last-wins when there is no INCRBY. Separately, StringBitPosition could not express an open-ended range. It always sent an explicit end, and for a clear-bit search the server distinguishes the two: with an explicit end an all-set string yields -1, whereas with no end the result is the first bit past the end of the string. Passing StringIndex.Unbounded as the end now omits it; that requires StringIndexType .Byte, since BYTE/BIT is only legal after an explicit end, and silently dropping the token would reinterpret start from bits to bytes. Implemented as a sentinel rather than an overload because a no-end overload would be CS0121-ambiguous with the existing one at every call site. * Name the unit, not the mechanism, on BitFieldOffset Bits/Index both said the wrong thing: Index named the mechanism ("an index") when the question is *of what* - bits or elements - and Bits(100) read as a quantity ("100 bits") rather than a position ("bit 100"). Singular noun plus ordinal, symmetrically: Bit(600) is bit 600, Element(3) is element 3, i.e. the #3 form the server multiplies by the width. Element rather than Field because "field" is already carrying weight in BitFieldEncoding and BitFieldOperation, and would read as naming a field rather than counting one. * Drop the meaningless outer null from the BITFIELD batch result The reply is always an array with one element per operation - empty for no operations - so a null *array* was unreachable; the only way to see one was a non-observable result such as fire-and-forget, where ExecuteSync hands back default(T). That made callers null-check for a state that never carries information, while the inner long? (an operation skipped by OVERFLOW FAIL) is the nullability that actually means something. Use the established defaultValue: idiom instead - as GeoPosition/GeoHash do for their arrays - so the batch returns a non-null Lease<long?> and yields an empty lease when there is nothing to observe. The single-operation overload keeps its long?, where null already means FAIL. * Write BITFIELD directly rather than building a RedisValue[] Follows the HotKeysStartMessage shape: a Message subclass that renders itself in WriteImpl, with ArgCount computed (and the payload validated) in the constructor, so a bad operation still throws at the caller rather than mid-write on the connection. This drops the intermediate RedisValue[] entirely - the batch form keeps the caller's ReadOnlyMemory, as the existing keyed-values messages do with caller arrays, and the single-operation form holds the operation inline, so that path now allocates nothing but the message. It also obviates the literals: the sub-command and overflow tokens are framed compile-time u8 spans written raw, which retires the RedisLiterals additions and the 128-entry encoding table behind ToLiteral. The encoding is written as one framed bulk string into a scratch buffer hoisted out of the loop: the width is 1-64, so the length prefix is always a single digit and it is either $2\r\nXX\r\n or $3\r\nXXX\r\n. The element offset shares that buffer for its '#' plus digits, but keeps WriteBulkString since its own length prefix varies. Round-trip tests now build the messages directly, as SetCardinalityRoundTrip does, which also retires the internal payload-builder seam that only existed for testability; new cases cover the encoding and offset framing at both prefix widths, ArgCount, and that a single operation writes byte-for-byte the same as a one-element batch. * Fail rather than guess in the BITFIELD writer The kind switch used its default arm for INCRBY, so an operation that was somehow neither GET nor SET would have been written as the most destructive of the three. Reject it instead: guessing is worse than failing here, because a wrong sub-command corrupts data and one of the wrong arity corrupts the connection - a GET written where the header promised four arguments leaves the server parsing the next command's bytes as arguments. The overflow arm keeps WRAP, and that asymmetry is deliberate: its argument count comes from the transition (op.Overflow != overflow) rather than the mode itself, so an unknown mode is shape-neutral and the server's own default is the harmless answer. CountArgs already rejected this at construction, but only watertight for the single-operation form, which copies. The batch form aliases the caller's memory, and unlike the messages holding a RedisValue[] - where ArgCount is the array length and content cannot change the shape - here the shape depends on the contents, so mutating the array between issue and write would desync the frame. Re-check before the header is written, so a violated contract fails with nothing written rather than corrupting the connection. * Keep the single-operation path actually allocation-free `[operation]` for a ReadOnlySpan parameter only avoids the heap where the by-ref span constructor exists. Checking the IL per framework: net8.0 and net10.0 emit ReadOnlySpan<T>::.ctor(!0&) over a stack local, but net461, netstandard2.0, net472 and net6.0 all emit `newarr BitFieldOperation` plus the array-based constructor - twice per call, once counting in the constructor and once writing - which is exactly the allocation that holding the operation inline was meant to avoid, on four of the six frameworks we ship. So don't take a span of operations at all on that path: the count and write steps become per-operation helpers taking `in BitFieldOperation`, and the batch form loops them over its span. The IL now shows no newarr in either message class on any framework, the only localloc being the intended scratch buffer. Also note in passing that the batch form's pre-write re-check becomes redundant once the write path serializes on the calling thread, since that removes the interval in which the aliased operations could be mutated at all. * Give an all-GET BITFIELD the read-only retry category Routing and replay safety are separate axes, and the fallback path conflated them. When every sub-operation is a GET but BITFIELD_RO is unavailable - a server below 6.0, or the command disabled or renamed in the command map - the message went out as BITFIELD and inherited its WriteAccumulating default, despite there being nothing to accumulate. The server insisting BITFIELD is a write governs which servers will accept it, not whether replaying it is safe. Apply CommandRetryReadOnly for any all-GET payload, whichever of the two commands we end up issuing; BITFIELD_RO already had it by default, so this only changes the fallback. An explicitly requested category still wins, as WithCategory has always done. The decision is now a pure static taking the availability as a parameter, which is the only way to observe it - the flags a message ends up with are not visible from outside - so all four payload shapes plus the explicit-category case are covered directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This simple code:
static void Main(string[] args)
{
var redis = ConnectionMultiplexer.Connect("localhost");
redis.Close();
redis.Close();
}
on second Close throws a NullReferenceException in StackExchange.Redis.ConnectionMultiplexer.OnCloseReaderWriter()