-
Notifications
You must be signed in to change notification settings - Fork 294
Add typed ClientChannelCloseException for client-rooted PUT termination #3280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
4a48efd
9e596b5
8730e80
c1d3aee
81e0af6
8123512
1438b79
d48baf9
4d97535
8165718
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,8 @@ | |||||||||||||||||||||
| import com.github.ambry.commons.Callback; | ||||||||||||||||||||||
| import com.github.ambry.router.AsyncWritableChannel; | ||||||||||||||||||||||
| import com.github.ambry.router.FutureResult; | ||||||||||||||||||||||
| import com.github.ambry.utils.ClientChannelCloseException; | ||||||||||||||||||||||
| import com.github.ambry.utils.PossibleClientChannelCloseException; | ||||||||||||||||||||||
| import io.netty.buffer.ByteBuf; | ||||||||||||||||||||||
| import io.netty.channel.Channel; | ||||||||||||||||||||||
| import io.netty.channel.DefaultMaxBytesRecvByteBufAllocator; | ||||||||||||||||||||||
|
|
@@ -65,6 +67,9 @@ public class NettyRequest implements RestRequest { | |||||||||||||||||||||
| // is <=0, it is assumed that there is no limit on the size of unacknowledged data. | ||||||||||||||||||||||
| static int bufferWatermark = -1; | ||||||||||||||||||||||
| private static final ClosedChannelException CLOSED_CHANNEL_EXCEPTION = new ClosedChannelException(); | ||||||||||||||||||||||
| private static final ClientChannelCloseException CLIENT_CHANNEL_CLOSE_EXCEPTION = new ClientChannelCloseException(); | ||||||||||||||||||||||
| private static final PossibleClientChannelCloseException POSSIBLE_CLIENT_CHANNEL_CLOSE_EXCEPTION = | ||||||||||||||||||||||
| new PossibleClientChannelCloseException(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| protected final HttpRequest request; | ||||||||||||||||||||||
| protected final Channel channel; | ||||||||||||||||||||||
|
|
@@ -301,6 +306,48 @@ public void close() { | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Marks this request's pending read (if any) as terminated because of a high-confidence, client-rooted event | ||||||||||||||||||||||
| * (e.g. the client disconnected or reset the connection). Must be called, if at all, before {@link #close()} so | ||||||||||||||||||||||
| * that {@link ClientChannelCloseException} - rather than the default {@link ClosedChannelException} - is delivered | ||||||||||||||||||||||
| * to the pending {@link #readInto} callback. Idempotent and safe to call even if there is no pending read. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| void markClientTerminated() { | ||||||||||||||||||||||
| channelException = CLIENT_CHANNEL_CLOSE_EXCEPTION; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+315
to
+317
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Realistic trigger: an undersized body is only detected on
Suggested change
Checking for the untouched sentinel rather than excluding one specific value keeps any real error that was already recorded, and stays correct if another writer to this field is added later.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks — I traced this end to end and I'd like to push back on the impact before changing the guard, because in the context of the goal here (separating sure-client vs possible-client vs server/unclassified connection closes) this guard is actually doing the right thing. Two findings:
Given that, I'd prefer to leave |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Convenience method that marks this request as client-terminated (see {@link #markClientTerminated()}) and then | ||||||||||||||||||||||
| * closes it, in one call. Use this at call sites that close the request directly (as opposed to sites where the | ||||||||||||||||||||||
| * close happens later via a different code path, e.g. through {@link NettyResponseChannel}), so the "mark before | ||||||||||||||||||||||
| * close" ordering requirement can never be broken by a future edit that reorders or drops one of the two calls. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| void closeDueToClientTermination() { | ||||||||||||||||||||||
| markClientTerminated(); | ||||||||||||||||||||||
| close(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Marks this request's pending read (if any) as terminated because of an event that is plausibly, but not | ||||||||||||||||||||||
| * confirmably, client-rooted (e.g. an idle timeout, which can equally be caused by a slow destination write; or an | ||||||||||||||||||||||
| * {@link java.io.IOException} reaching Netty's {@code exceptionCaught}, which is usually but not provably | ||||||||||||||||||||||
| * client-facing). Must be called, if at all, before {@link #close()} so that | ||||||||||||||||||||||
| * {@link PossibleClientChannelCloseException} - rather than the default {@link ClosedChannelException} - is | ||||||||||||||||||||||
| * delivered to the pending {@link #readInto} callback. Does not overwrite an already-set | ||||||||||||||||||||||
| * {@link #markClientTerminated() high-confidence} tag, so that a "sure" classification is never downgraded to | ||||||||||||||||||||||
| * "possible" if both were somehow triggered for the same request. Idempotent and safe to call even if there is no | ||||||||||||||||||||||
| * pending read. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| void markPossibleClientTermination() { | ||||||||||||||||||||||
| // Check-then-act, not atomic/CAS - safe because channelInactive/exceptionCaught/userEventTriggered all fire on | ||||||||||||||||||||||
| // the same Netty channel's single-threaded event loop for a given request, so these calls are never actually | ||||||||||||||||||||||
| // concurrent with each other; the volatile field just ensures the eventual invokeCallback() on another thread | ||||||||||||||||||||||
| // sees the final write. | ||||||||||||||||||||||
| if (channelException != CLIENT_CHANNEL_CLOSE_EXCEPTION) { | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same issue as
Suggested change
Checking against the default sentinel ("has anything meaningful been written here yet?") rather than excluding one specific value covers both cases, and stays correct if another writer to this field is added later. Worth a test where
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed the guard is imprecise, but I checked reachability and I don't think a For validateState's
Functionally it also has no effect on the tiering: "possible" is already the lowest live tier, and the |
||||||||||||||||||||||
| channelException = POSSIBLE_CLIENT_CHANNEL_CLOSE_EXCEPTION; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @Override | ||||||||||||||||||||||
| public RestRequestMetricsTracker getMetricsTracker() { | ||||||||||||||||||||||
| return restRequestMetricsTracker; | ||||||||||||||||||||||
|
|
@@ -336,7 +383,14 @@ public Future<Long> readInto(AsyncWritableChannel asyncWritableChannel, Callback | |||||||||||||||||||||
| try { | ||||||||||||||||||||||
| if (!isOpen()) { | ||||||||||||||||||||||
| nettyMetrics.requestAlreadyClosedError.inc(); | ||||||||||||||||||||||
| tempWrapper.invokeCallback(new ClosedChannelException()); | ||||||||||||||||||||||
| // Deliver the stored channelException (not a fresh ClosedChannelException) so a client-termination | ||||||||||||||||||||||
| // classification recorded via markClientTerminated()/markPossibleClientTermination() before close() is | ||||||||||||||||||||||
| // preserved even when the read is registered AFTER the request already closed. This is the realistic | ||||||||||||||||||||||
| // AsyncRequestResponseHandler queue-then-read ordering: a client can disconnect (channelInactive -> | ||||||||||||||||||||||
| // closeDueToClientTermination) before the router calls readInto(), at which point close() had no | ||||||||||||||||||||||
| // callbackWrapper to deliver to. channelException defaults to a plain ClosedChannelException when the | ||||||||||||||||||||||
| // request was never tagged, so the untagged path is unchanged. | ||||||||||||||||||||||
| tempWrapper.invokeCallback(channelException); | ||||||||||||||||||||||
| } else if (writeChannel != null) { | ||||||||||||||||||||||
| throw new IllegalStateException("ReadableStreamChannel cannot be read more than once"); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but this drops the useful part of the diagnosis.
causehere carries the actual socket message ("Connection reset by peer","Broken pipe"), and thereadIntocallback ends up with a cause-less, message-less singleton instead.It's also asymmetric:
onRequestAborted((Exception) cause)a few lines below still forwards the real exception to the response path, so whether the underlying reason is visible depends on which side you're reading. During an incident that's exactly the detail you want.Suggest an overload that preserves it —
ClosedChannelExceptionhas no cause constructor, so it needsinitCause, which also means not using the shared singleton on this path:The no-arg version can stay as-is for the idle-timeout call site, which genuinely has no cause to attach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that the asymmetry is real — the
readIntocallback gets the message-less singleton whileonRequestAborted((Exception) cause)forwards the real socket exception — and your point thatinitCauseneeds a fresh instance (the singleton is shared/class-loaded) is correct.That said, for the classification goal this is observability-only, so I'm inclined to leave it:
RouterUtils.isSystemHealthError()andUtils.isPossibleClientTermination()both classify purely byinstanceof, so the tier is already correct (possible) with or without the cause attached.onResponseComplete'slog(exception), which receives the actualcause. Only the router-side log line is less specific.So the enrichment is a genuine nicety but doesn't change how any close is tiered. If diagnosing these on the router side turns out to be painful in practice I'm happy to add the
markPossibleClientTermination(Throwable cause)overload (fresh instance +initCause, no-arg kept for the idle path) as a follow-up — but I'd keep it out of this PR since it's not part of the sure/possible/server separation.