Fix that TlsHandler don't run well on Mono #374
Conversation
yyjdelete
changed the title
yyjdelete
changed the title
yyjdelete
force-pushed
the
tls
branch
4 times, most recently
from
492f893 to
d2893d1
Compare
|
It's strange that The ci test always failed(but not failed in local) yesterday after this commit and later But the same commit always success today. Feel free to revert it if that happened again. |
yyjdelete
changed the title
| fixed (byte* source = &src[srcIndex]) | ||
| fixed (byte* destination = &dst[dstIndex]) | ||
| Unsafe.CopyBlock(destination, source, unchecked((uint)length)); | ||
| Unsafe.CopyBlockUnaligned(ref dst[dstIndex], ref src[srcIndex], unchecked((uint)length)); |
There was a problem hiding this comment.
Unsafe does a buffer pin/unpin, for unsafe buffers (already pinned on the arena), this is extra performance overhead
You have to do something like
IntPtr ptr = dst.AddressOfPinnedMemory();
if (ptr != IntPtr.Zero)
{
PlatformDependent.CopyMemory(addr, (byte*)(ptr + dstIndex), length);
}
else
{
fixed (byte* destination = &dst.GetPinnableMemoryAddress())
{
PlatformDependent.CopyMemory(addr, destination + dstIndex, length);
}
}
That is the reason why PlatformDependent does not use the ref param methods.
There was a problem hiding this comment.
I didn't recognized it's already pinned in DirectArena.MemoryChunk before.
But I think maybe ref is no need to be pinned, and this commit didn't change any overload with an pointer as param to ref. And the only thing need to be change is UnsafeByteBufferUtil.Copy, which is out of scope of this PR.
Maybe I will do some test to see the difference latter.
There was a problem hiding this comment.
@yyjdelete ref does a pin/unpin (fixed) for sure, your changes are ok as is. I plan to consolidate all unsafe calls.
There was a problem hiding this comment.
@StormHub so this is good with you?
|
@nayato unsafe unaligned is good on netcore 2.1 preview. Eventually Span is a better choice once we move to 2.1. |
|
@nayato 5370c02 override all methods in LoggingHandler And some thing in |
|
@yyjdelete that would be great. Feel free to bundle. |
* Add some missing method to LoggingHandler * Avoid to alloc an huge error message when the test not failed. * Update the unittest * Update Microsoft.NET.Test.Sdk from 15.0.0 to 15.7.2, fix that unable to debug an unittest for the second time. * Disable parallelization for InternalLoggerFactoryTest.TestMockReturned to avoid an rare test failure. * Remove `dotnet-xunit` since it's never used and will be discontinued, see https://xunit.github.io/releases/2.4-beta2 * Remove space from filename * Switch back to `DiscardSomeReadBytes` since it's avaliable * Rework some logic in TlsHandler * Make sure TlsHandler.MediationStream works well with different style of aync calls(Still not work for Mono, see #374) * Rework some logic in #366, now always close TlsHandler.MediationStream in TlsHandler.HandleFailure since it's never exported. * Workaround to fix issue 'microsoft/vstest#1129'. * Change the default of TcpServerSocketChannel.Metadata.defaultMaxMessagesPerRead to 1
* Add some missing method to LoggingHandler * Avoid to alloc an huge error message when the test not failed. * Update the unittest * Update Microsoft.NET.Test.Sdk from 15.0.0 to 15.7.2, fix that unable to debug an unittest for the second time. * Disable parallelization for InternalLoggerFactoryTest.TestMockReturned to avoid an rare test failure. * Remove `dotnet-xunit` since it's never used and will be discontinued, see https://xunit.github.io/releases/2.4-beta2 * Remove space from filename * Switch back to `DiscardSomeReadBytes` since it's avaliable * Rework some logic in TlsHandler * Make sure TlsHandler.MediationStream works well with different style of aync calls(Still not work for Mono, see Azure#374) * Rework some logic in Azure#366, now always close TlsHandler.MediationStream in TlsHandler.HandleFailure since it's never exported. * Workaround to fix issue 'microsoft/vstest#1129'. * Change the default of TcpServerSocketChannel.Metadata.defaultMaxMessagesPerRead to 1
|
Hi, The changes in the PR to TlsHandler.cs address this issue, I understand this may be a Mono problem underneath, but I was wondering what the state of this was and if it was to be merged? The original problem by the way was a hang on IChannelHandlerContext.WriteAndFlush, eventually giving a ClosedChannelException Thanks in advance, |
|
Tested on net5.0-preview4+win10x64 without See another similar problem on net5.0+win10x64(also known as netcoreapp5.0) , and this patch(only Not test on net5.0+linux. I will rebase this PR on the top of dev latter, in case of someone still need this. For 331ea1e, view with |
…hich is not read. Maybe needed for mono(old version only?) and net5.0(without NETSTANDARD1_3 defined)
In Mono(with btls provider) on linux, and maybe also for apple provider, Write is called in another thread, so it will run after the call to Flush.
`try{_=task.Result;}catch(AggregateException ex){ExceptionDispatchInfo.Capture(ex.InnerException).Throw();/*unreachable*/throw;}` => `task.GetAwaiter().GetResult()`, the latter one will not wrap Exception as AggregateException.
…ix concurrent issue for Wrap
No description provided.