(dev/eng) Support transport feeds - #3173
Merged
Merged
Conversation
…nsportAsync (SER009) When a Tunnel yields a DuplexTransport, PhysicalConnection now runs with no socket, no Stream, no SslStream and no reader thread: the transport is acquired BEFORE socket creation (the widest form of the existing connectTo=null no-socket pattern), outbound rides the existing _output seam through a TransportWriter : BufferedStreamWriter adapter (the transport IS an IBufferWriter with explicit Flush, so every call site is untouched), and inbound is PUSH -- TransportFeed commits transport-owned spans into the same CycleBuffer + CommitAndParseFrames the pull loops use, on the transport's threads. One copy, zero thread hops, no read syscall surface on this side. TLS contract: a transport tunnel owns TLS; config.Ssl=true alongside one throws NotSupportedException rather than silently double-encrypting or silently skipping. Teardown disposes the transport in Shutdown alongside socket/output, and Dispose treats an owned transport like an owned socket. Gated end-to-end from the SocketSet side (bench/tunnel-selftest, mux + mux-tls cells): a real ConnectionMultiplexer over SocketSetTunnel against Garnet -- connect (HELLO handshake included), PING, SET/GET, 500-op pipelined burst, plaintext AND transport-terminated TLS, ALL PASS first run. The 129-test writer/round-trip/in-proc battery still passes (classic paths untouched).
…ee (SER009) A transport tunnel owns connect and TLS end-to-end, so two things were wrong: it could not SEE the TLS configuration (the same engine may be run with or without TLS - that cannot be assumed either way), and ConnectedAsync prejudiced against config.Ssl by throwing outright. Ssl=true is entirely valid in transport mode; the real question is whether the TRANSPORT disagrees. ConnectTransportAsync now takes a TlsOptions: a readonly struct wrapping the ConfigurationOptions, whose accessors proxy through to just the TLS-relevant settings (IsEnabled, SslHost, SslProtocols, CheckCertificateRevocation, the certificate validation/selection callbacks including their ambient env-var fallbacks, GetSslClientAuthenticationOptions on net6+, and ResolveHost, which applies the same SslHost-or-endpoint-host fallback our own TLS path uses). Zero-alloc, nothing snapshotted, and it cannot be used to reach - or change - anything else about the configuration. Backend-specific settings remain the tunnel constructor's business. DuplexTransport gains IsEncrypted (virtual, default false: the safe answer, so a TLS-terminating transport that forgets to override is refused rather than trusted). ConnectedAsync now fails only on actual disagreement - config.Ssl with a transport reporting plaintext - recorded as AuthenticationFailure like the SslStream path, not the generic InternalFailure catch. The converse (encrypted when the config did not demand it) is accepted and logged: more secure than asked for is fine. On the "actively assert ssl.IsEncrypted after the handshake" advice for the pre-existing SslStream path: it is moot, and the comment at the site now says so with both source links. On every TFM we target, SslStream.IsEncrypted (and IsSigned) is an alias for IsAuthenticated - "handshake completed, no exception" - which a successful await already proved; it says nothing about the bytes on the wire. What forbids plaintext there is the EncryptionPolicy.RequireEncryption we already pass. Rather than a tautological assert that implies a guarantee it does not make, net6+ now logs the NegotiatedCipherSuite alongside SslProtocol, so a surprising negotiation is visible. Two rough edges found while in here: LoggingTunnel never forwarded ConnectTransportAsync to its tail, so wrapping a transport tunnel silently bypassed the transport and fell back to a socket. It now forwards and wraps via a new virtual Log(DuplexTransport, ...) hook - LoggingDuplexTransport captures into the same .in/.out pair the stream path produces, so ReplayAsync/ValidateAsync read both unchanged. Outbound is captured in Advance (the last point the bytes are still ours; hence GetSpan routes through our GetMemory), inbound before the consumer sees it, and IsEncrypted forwards to the inner transport. Since our constructor clears Ssl from the live options so captures are plaintext, the tail is handed a Clone with the original intent restored - otherwise a logged transport would have quietly connected in the clear. The default hook returns the transport unchanged: a custom subclass that only implements stream logging connects unlogged rather than failing the connection. TransportFeed did not implement OnBatchEnd, which the contract asks for explicitly. TransportWriter now tracks bytes advanced since the last flush (approximate in the safe direction: a redundant flush attempt, never a missed one) and the callback flushes through PhysicalBridge.TryFlushStagedWrites, which takes the write lock with TryTakeInstant or does nothing at all. The lock is what makes staging exclusive, so an unguarded flush could hand a half-written frame to a transport mid-Advance; instant-or-nothing also declines safely if the burst is being pumped inline on a thread that already holds it. The only thing that can be staged there is an inbound-triggered write that did not demand a flush, so the common path is one volatile read. Solution builds clean on all TFMs (warnings-as-errors). net10.0 suite: 5666 pass / 8 fail, against 6 failures on a HEAD baseline in a temporary worktree - a disjoint set of names, all connect-timeout shaped, from running against a trimmed local server topology; each of the 8 passes in isolation. Note that transport mode itself has no test coverage in this repo at all, so the above is compile-and-review verified, not exercised.
# Conflicts: # src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt
…t TLS Framework convention is Pascal casing for acronyms of three or more letters (only two-letter ones stay upper: IPEndPoint, IOException), and the BCL is consistent about this exact acronym: SslStream, SslProtocols.Tls13, SslClientAuthenticationOptions, TlsCipherSuite. The repo already leaned that way - TlsHandshakeAsync, and the new TlsOptions - leaving these two internal LoggerMessage partials as the only TLS-cased identifiers in src. Internal, so this is rename-only: no API file, no compat surface, six call sites. Prose is untouched - "Configuring TLS" in a log message is English, where the upper-case acronym is correct.
The experimental gate exists to stop *randoms* leaning on an unstable transport seam, not to make our own consuming code apologise for itself in every file - and SER001/004/005/007/008 are already handled this way in the root props, so SER009 was the odd one out. That removes the file-level disables in PhysicalConnection.Transport.cs and LoggingTunnel.cs and the disable/restore pair around the ConnectTransportAsync call site; the [SER009] prefixes in the PublicAPI files are unrelated and stay. Also: both files this branch introduced were missing the utf-8-bom the .editorconfig asks for.
LoggingTunnel needed the tail to see TLS after its own constructor cleared Ssl from the live options, and cloning ConfigurationOptions per connect attempt to carry one bit was a poor trade. TlsOptions now has a private _forceTls alongside the wrapped options, a private (options, forceTls) constructor, and an internal WithTls() - so the call site is one line and allocation-free: `if (_ssl) tls = tls.WithTls();`. IsEnabled ORs the flag over the configuration; every other accessor is untouched, since only the intent differs, never the host/protocols/callbacks. The public constructor chains to the private one (with the null check as a throw expression) rather than assigning a subset and letting the compiler zero-init the rest via an implicit : this(). Internal member and private constructor, so no PublicAPI change. Builds clean on all TFMs with RunAnalyzers=true; Parse/Config/SSL batteries pass (310).
The generated half settles it - the source generator emits new EventId(<id>, nameof(<Method>)), so the
method name lands in EventId.Name and reaches consumers as the event name ("EventName" in most structured
sinks), where it can be filtered or alerted on. Nothing is free at 8M+ downloads a day, so the awkward
spelling stays: LogInformationConfiguringTLS and LogInformationTLSConnectionEstablished are back, with a
note at the top of the generated section stating that renaming these is a telemetry change rather than a
refactor (and that EventName can pin an old name if one ever must move), plus a marker at the two
TLA-offending declarations so nobody tidies them again.
While applying the same reasoning to the code added on this branch: the net6+ cipher-suite variant now
declares EventId 99 - the same event as its sibling, one logical "TLS connection established" carrying an
extra property where the platform can report it - instead of minting 110 and thereby reporting the same
event under different ids on netfx and modern TFMs. The generator is happy to share an id across
overloads; verified in the emitted source. LogInformationTransportConnected takes 110, and since both of
those ids are new on this unshipped branch, no consumer ever saw either.
Builds clean on all TFMs with RunAnalyzers=true; Config/SSL/Parse batteries pass (310).
mgravell
added a commit
that referenced
this pull request
Aug 14, 2026
Two CI changes, both for the same reason: the Windows leg failed and the detail was unreachable. dorny/test-reporter defaults to listing every suite and every test, which rendered ~3MB into $GITHUB_STEP_SUMMARY against a 1MB limit - so the summary was dropped wholesale and the actual failures had to be read out of the check-run annotations API. list-suites/list-tests: failed keeps full detail for failures and collapses the passing thousands to counts, which also means a failure is no longer buried in a wall of green. Note this was never the xunit logger's doing: its summary is a fixed counts table, so suppressing passed/skipped there changes nothing. Also uploads the trx (14-day retention). The run had no artifacts at all, and the failure-only report deliberately omits per-test output - which is precisely what a CI-only failure needs. PublicAPI.Unshipped.txt conflicted again. Resolved by taking main's content verbatim and appending ours, rather than merging into sorted order as before: main appends to the end of its own list, so a sorted file interleaves the two and conflicts on every merge. Dropped our now-stale Tunnel.ConnectTransportAsync entry, which #3173 replaced with the TlsOptions overload.
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
1 participant
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 is background prep work related to the
SocketSetnetwork core for a hypothetical future networking implementation based on io_uring, iocp, etc using dedicated mass-socket APIs; there are no direct dependencies, instead we provide a suitable push-transport API in SE.Redis with no concrete implementation, and a hypotheticalSocketSet.StackExchange.Rediscan implement that side via theTunnelhook.For now: this is entirely unused and inert.
Checklist