Conversation
…ionFilter
Previously, `FilterStack` and `CompressionFilter` always executed
`sendMetadata`, `sendMessage`, and `receiveMessage` through `async` functions
and `Promise` chains—even when no compression was used and no asynchronous
filters were present in the stack. As a result, every uncompressed RPC had to
allocate multiple intermediate `Promise`s and closures and hop through the
microtask queue for metadata filtering, request framing, and response
deframing.
This change adds an optional synchronous fast path (`sendMetadataMaybeSync`,
`sendMessageMaybeSync`, and `receiveMessageMaybeSync`) to `Filter`,
`FilterStack`, `CompressionFilter`, and `ResolvingCall`.
- **Synchronous metadata and uncompressed message framing/deframing**:
`CompressionHandler.writeMessage` and `readMessage` now return a `Buffer`
synchronously when no compression or decompression is required (such as
`identity` encoding or when `WriteFlags.NoCompress` is set), only returning
a `Promise` when `zlib` compression or decompression is actually needed.
- **Zero-allocation pass-through for `BaseFilter` methods**: `FilterStack`
skips methods inherited unchanged from `BaseFilter` (such as pass-through
methods on `RouterFilter`) without allocating a `Promise`.
- **Shared `IDENTITY_HANDLER` singleton**: `IdentityHandler` has no instance
state, so `CompressionFilter` now reuses a module-level singleton instead of
allocating two new `IdentityHandler` instances per call.
- **Synchronous child call start on warm channels**: When channel configuration
is already resolved and metadata filters are synchronous, `ResolvingCall` now
starts the child `RetryingCall` and forwards the initial request message and
`halfClose` in the same turn, avoiding the intermediate `pendingMessage`
queuing in `ResolvingCall` and allowing `LoadBalancingCall` to flush the
initial message and half-close together when call credentials resolve.
- **Full backward compatibility for existing filters and callers**: The
existing `sendMetadata`, `sendMessage`, and `receiveMessage` methods on
`Filter`, `BaseFilter`, and `FilterStack` are preserved. When a legacy filter
is present in the stack, `FilterStack` seamlessly transitions to `Promise`
chaining for that filter and any subsequent filters in the pipeline.
- **Unchanged ordering and async guarantees**: When any filter in the stack
returns a `Promise` (for example, `gzip` or `deflate` compression, or a
custom async filter), `ResolvingCall` continues to set `writeFilterPending`
and `readFilterPending` to defer `halfClose` and `onReceiveStatus` until the
pending filter finishes. Status delivery in `outputStatus` continues to use
`process.nextTick`.
- **Minor housekeeping**:
- Clear `this.pendingMessage = null` in `ResolvingCall` and
`LoadBalancingCall` once the queued message has been forwarded to the child
call so the request buffer is not retained for the rest of the call.
- Check `if (this.ended) return;` when asynchronous metadata or message
filters resolve so cancelled calls do not start a child call or forward
in-flight messages after cancellation.
murgatroid99
left a comment
murgatroid99
left a comment
There was a problem hiding this comment.
The *MaybeSync methods you added here are effectively strictly more general than the existing methods, and the Filter and FilterStack APIs are experimental, meaning that breaking changes can be made to them in new minor versions. In addition, having both and making the *MaybeSync methods optional increases the complexity of the new code. So, I would prefer to just replace the existing methods. That would require changes to the other filters, and to packages/grpc-js-xds/src/http-filter/fault-injection-filter.ts, but they should be pretty trivial.
| result = | ||
| result instanceof Promise | ||
| ? result.then(resolvedMetadata => | ||
| filter.sendMetadataMaybeSync!(resolvedMetadata) | ||
| ) | ||
| : filter.sendMetadataMaybeSync(result); |
There was a problem hiding this comment.
The result here should also need the same normalization mentioned in the comment in the last case, because it can similarly return a Promise<Metadata>. However, we can't just unconditionally wrap the result in Promise.resolve here, because that would reverse most of the gains we get from using Metadata objects directly. Also, any check for whether the result needs to be normalized could instead be used in place of the result instanceof Promise line here.
I think the actual solution here is to replace result instanceof Promise with a more generic isThenable(result) check. The same applies to the other similar functions.
…ter API Address review feedback on the synchronous filter fast path. Instead of adding separate *MaybeSync methods next to the existing Promise-based ones, the Filter methods themselves now take a plain value and may return either the result directly or a thenable when they need to do asynchronous work. BaseFilter's methods are now simple synchronous pass-throughs. Filter results are now detected with an isThenable check instead of `instanceof Promise`, so the result of every filter is handled the same way, including the last one in the stack. Promises from other realms and custom thenables are handled correctly as well. The special case that skipped BaseFilter pass-through methods in FilterStack has been removed, as it is no longer needed now that those methods are synchronous. This is a breaking change to the experimental Filter API: filters now receive the value itself instead of a Promise of it. The fault injection filter in grpc-js-xds has been updated to match. Filters that simply `await` their input keep working.
ddd50c7 to
f66fcd0
Compare
Done |
3 participants
Summary
Filters no longer have to return a Promise. If a filter can do its work
right away, it just returns the result, and the call keeps going without
waiting for a microtask. Only filters that actually do async work, like
gzip compression or xds fault injection, still return a Promise.
This changes the experimental Filter API: filters now receive a plain
value instead of a Promise. The xds fault injection filter is updated
accordingly. Existing filters that just
awaittheir input keep working.Long Version
Previously, FilterStack and CompressionFilter always ran sendMetadata,
sendMessage and receiveMessage through async functions and Promise
chains, even when no compression was used and no asynchronous filter
was present. Every uncompressed RPC therefore allocated several
intermediate Promises and closures and went through the microtask queue
for metadata filtering, request framing and response deframing.
The Filter methods sendMetadata, sendMessage and receiveMessage now take
their input value directly and return either the result or a thenable
(T | PromiseLike). This is a change to the experimental Filter API.
Filters that simply
awaittheir input keep working.CompressionFilter sets its headers synchronously, and
CompressionHandler.writeMessage and readMessage return a Buffer
directly when no compression or decompression is needed (identity
encoding, or WriteFlags.NoCompress). A Promise is only returned when
zlib is actually used.
plain pass-throughs, so filters that don't override a method add no
allocations.
a filter returns a thenable. Results are detected with an isThenable
check, so promises from other realms and custom thenables work too.
CompressionFilter reuses one module-level instance instead of
allocating two per call.
config is already resolved and metadata filters are synchronous,
ResolvingCall starts the child call and forwards the first message
and halfClose in the same turn.
Behavior is otherwise unchanged:
async filter such as xds fault injection), ResolvingCall still sets
writeFilterPending and readFilterPending to hold halfClose and
onReceiveStatus until the filter finishes. Status is still delivered
via process.nextTick in outputStatus.
keeps its existing behavior. RouterFilter needs no changes.
Minor housekeeping:
the queued message has been forwarded, so the request buffer is not
kept for the rest of the call.
if (this.ended) return;when asynchronousmetadata or message filters resolve, so a cancelled call does not
start a child call or forward in-flight messages.