Skip to content

Fix: if calls Close or Dispose many times it'll throws a NullReferenceException - #3

Merged
mgravell merged 1 commit into
StackExchange:masterfrom
matteobaglini:master
Mar 22, 2014
Merged

mgravell merged 1 commit into
StackExchange:masterfrom
matteobaglini:master

Conversation

@matteobaglini

Copy link
Copy Markdown
Contributor

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()

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
@mgravell
mgravell merged commit 7c85913 into StackExchange:master Mar 22, 2014
@mgravell

Copy link
Copy Markdown
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.
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