Include an error message payload alongside ChannelTerminated frames #536
Conversation
| if (ex is OperationCanceledException && this.DisposalToken.IsCancellationRequested) | ||
| { | ||
| await mxStreamIOReader!.CompleteAsync().ConfigureAwait(false); | ||
| traceMessage = string.Format("Swallowed exception inside ProcessOutboundAsync: {0}", ex.Message); |
There was a problem hiding this comment.
I'm confused as to why you wrote this. Swallowing an exception means we don't re-throw it. But we do (still) rethrow it below, at the end of the catch block. It looks like you were saying this because you wouldn't be forwarding this exception to the remote party, so maybe say that? But that said, we should be careful about swallowing anything, because even an OperationCancelledException may be something we need to forward to the remote party. Really, anything that could possibly cast doubt on the quality of the data (including possible truncation) should be communicated as an exception.
And BTW, we should always specify a CultureInfo as a first argument to string.Format. For trace messages, using CultureInfo.InvariantCulture is probably appropriate.
There was a problem hiding this comment.
For the second point that you made, that makes sense. For the first point that you made, I remember us discussing at the start of the project that we don't want to transmit errors where ex is OperationCanceledException && this.DisposalToken.IsCancellationRequested to the remote side which is why I introduced that. For now, I have gotten rid of that if statement and moving from the else statement into the catch block.
| if (this.TraceSource!.Switch.ShouldTrace(TraceEventType.Information)) | ||
| { | ||
| this.TraceSource.TraceEvent( | ||
| TraceEventType.Information, |
There was a problem hiding this comment.
Using Information level messages to communicate a TraceEventId.FatalError seems ironic. But this isn't a fatal error. At least, IIRC, the FatalError code indicates the MultiplexingStream as a whole is going down, but it isn't. Just this channel is. Maybe we should define a new ChannelFatalError code? And then use an TraceEventType.Error level for it here.
There was a problem hiding this comment.
Does it make sense to introduce the ChannelFatalError code for just this one case. I am not sure what you generally consider a valid enough threshold for introducing a new code. Till then I have changed it to be of error level fatal and a code of zero.
sarda-devesh
force-pushed
the
errorWithTerminate
branch
from
31ae329 to
0d19c1d
Compare
AArnott
force-pushed
the
errorWithTerminate
branch
from
4dcce6a to
2dd2f9a
Compare
AArnott
changed the title
AArnott
force-pushed
the
errorWithTerminate
branch
from
2dd2f9a to
7284090
Compare
The new feature added in #536 that communicates channel failures to the remote party also caused channels to report failure if their owner `MultiplexingStream` was disposed of before the channel was. This broke several tests in the vs-servicehub repo and could theoretically break shipping code as well. In this change I hide this particular behavioral change behind a setting that requires opt-in.
Closes #299