Defer deserialization of message body until access time
- Dominant language
- C#
- Stars
- 10.9k
- Forks
- 2.1k
- Avg merge
- 15h 1m
- Merged PRs (30d)
- 345
Description
When we migrated to the Bedrock interfaces (i.e, Kestrel's networking abstractions), we abandoned our buffer pooling and instead used the new shared buffer pooling infrastructure (`System.Buffers.MemoryPool.Shared`, for example).
This was a known compromise, since our existing buffer pooling allowed for finer-grained release semantics, so that we could release the headers back to the pool immediately while preserving the body of the message to be released back to the pool later: a useful trait for zero-copy networking. It also had benefits, though. In particular, it was much simpler since we did not have to closely track message object lifetimes.
In making the move to Bedrock, we switched from deferred deserialization of message payloads to eager deserialization. That means that the body of a message (eg, the request/response data) is always deserialized as soon as it's received. This can cause issues for clusters with uncoordinated or rolling upgrades. For example, an upgraded caller might send an incompatible message to a non-upgraded host, resulting in an error.
**This issue tracks re-implementing deferred deserialization of the message payload.**
The primary complication is that message lifecycle is not fixed or certain. Messages come in several flavors (Request/Response/OneWay, for example) and have a variety of origins and destinations, eg:
* To/from a remote host (represented by the `Connection` class)
* To/from a grain (`IGrainContext`), sent request messages are logically owned by a `CallbackData` object, and received messages are owned by the grain/connection which receives them
* Dropped at some point due to expiry or overload
Messages may also be accessed asynchronously: the `IncomingRequestMonitor` scans for requests which have been queued or active for a long time and accesses them in the background. Solutions which allow a message to be disposed (buffers returned to pool) while it's being inspected by this monitor could result in an error.
Therefore, I believe reference tracking for `Message` objects is the right approach. Here are some pros and cons. You could probably come up with others:
**Cons:**
It's costly in terms of code complexity and comes with the risk of memory leaks due to bugs as well as the possibility of memory usage bloat if there are many concurrent messages which are holding onto larger-than-necessary buffers. It also requires copying network buffers into buffers rented from the memory pool separately.
**Pros:**
On the positive side, it enables us to implement some performance optimizations unrelated to this issue, such as pooling `Message` objects and bodies and can also improve locality of the deserialized message payload (which can help allocated objects to remain in Gen0).
@benjaminpetit reminded me of one nefarious and difficult to track bug in our previous implementation of this, which was essentially a double-free bug which lead to memory corruption. One way to reduce the chance of that would be to make `Message` objects responsible for tracking their buffers, so that they can hold the reference count (previously, we had no reference count).
An alternative to ref counting could be to perform deserialization of buffers under a lock, releasing the buffers once deserialization is complete. This still requires some form of lifecycle tracking, since the buffers *must* be released even if deserialization never occurs, otherwise there would be a memory leak.
My initial attempt at describing how the system will track message lifecycle is as follows:
* `Message` is created with a reference count of 1. Therefore, a single `message.Release()` call will return its buffers to the pool.
* `CallbackData` is used to track any request originating from the current host
* It calls `message.TryPreserve` on creation. If it returns false, throw (this is a debug assertion, it should never happen)
* On termination (timeout, completion), it calls `message.Release()`
* `Connection` and derived classes take ownership of any outbound `Message` object which they are given via `Send`, and must therefore ensure that they call `Release()` on each once they are done with it, unless they transfer ownership to another object.
* Any time a message is dropped due to expiry or another reason, `Release()` must be called
* Any time a response message is created for a message, `Release()` must be called.
* When a message is passed to a grain via `IGrainContext.ReceiveMessage`, it takes ownership of that message and must therefore eventually call `Release()` on it, unless it transfers ownership to another object.
To restate that in more implementable terms:
* `Message` is created with a reference count of 1
* `Message.Release()` decrements the count and returns its buffers to the pool if the call results in the reference count hitting zero
* `Message.TryPreserve()` increments the count only if is currently greater than zero, returning true if it was incremented and false otherwise. i.e, you cannot call `TryPreserve()` on an already-disposed message.
* `CallbackData` calls `message.TryPreserve()` on creation and `Release()` when the request terminates (guaranteed to happen eventually)
* `IncomingRequestMonitor` calls `message.TryPreserve()` prior to inspection and `message.Release()` afterwards
* `Release()` must also be called:
* When a response (success or otherwise) is created for a message
* When a message is dropped
* When a `Connection` sends a message to a remote host
* Message releases its buffers when either the reference count hits zero or the payload is accessed (in which case the buffers will be replaced by the deserialized payload)
In the future, to support pooling of message object themselves while detecting/preventing use-after-free errors, we could adopt a pattern similar to `ValueTask`+`IValueTaskSource`, whereby `ValueTask` tracks the (poolable) `IValueTaskSource` with a numeric cookie, throwing if its cookie value doesn't match the `IValueTaskSource`'s cookie value. That's not necessary for this issue, though.
Feedback always welcome
Contributor guide
Assessment
This issue has not been assessed yet.