Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Additional logging for errors in ByteBufferAsyncProcessor #190

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Additional logging for errors in ByteBufferAsyncProcessor #190

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 25 additions & 17 deletions rd-net/Lifetimes/Threading/ByteBufferAsyncProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,30 +483,38 @@ [PublicAPI] public void Put(byte* start, int count)
// return;
// }

var ptr = 0;
while (ptr < count)
try
{
Assertion.Assert(myChunkToFill.IsNotProcessed, "myChunkToFill.IsNotProcessed");
var rest = count - ptr;
var available = ChunkSize - myChunkToFill.Ptr;
if (available > 0)
var ptr = 0;
while (ptr < count)
{
var copylen = Math.Min(rest, available);
Marshal.Copy((IntPtr)(start + ptr), myChunkToFill.Data, myChunkToFill.Ptr, copylen);
myChunkToFill.Ptr += copylen;
ptr += copylen;
Assertion.Assert(myChunkToFill.IsNotProcessed, "myChunkToFill.IsNotProcessed");
var rest = count - ptr;
var available = ChunkSize - myChunkToFill.Ptr;
if (available > 0)
{
var copylen = Math.Min(rest, available);
Marshal.Copy((IntPtr) (start + ptr), myChunkToFill.Data, myChunkToFill.Ptr, copylen);
myChunkToFill.Ptr += copylen;
ptr += copylen;
}
else
{
GrowConditionally();
myChunkToFill = myChunkToFill.Next;
}
}
else

if (myAllDataProcessed) //speedup
{
GrowConditionally();
myChunkToFill = myChunkToFill.Next;
myAllDataProcessed = false;
Monitor.Pulse(myLock);
}
}

if (myAllDataProcessed) //speedup
catch (Exception ex)
{
myAllDataProcessed = false;
Monitor.Pulse(myLock);
LogLog.Error(ex);
throw;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Note that rethrowing with “throw;” instead of “throw e;” would “preserve the corrupting-ness”, meaning the whole process would probably be torn down as the re-thrown exception continued to propagate.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hypersw suggests to use throw new Exception(e) to preserve the original callstack

Copy link
Collaborator Author

@ForNeVeR ForNeVeR Dec 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case, I don't think that's necessary.

Here, I want the following behavior from this code:

  1. If any exception (including the corrupted state one) is thrown, then the lock should be unlocked, and the exception should be logged without reentrancy into this code region, if possible (LogLog allows such logging).
  2. If the caller of this code uses [HandleProcessCorruptedStateExceptions], then let it to handle the exception as it wants to.
  3. If the caller of this code doesn't use [HandleProcessCorruptedStateExceptions], then fall back to the default behavior (say, crash the whole process).

My point is that the logger internals shouldn't modify the defaults chosen by the application domain policy or by the caller. I don't think that this code should care about transforming corrupted state exceptions into ordinary ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After an internal discussion, we've decided to make this behavior configurable.

}
}
}
Expand Down