-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Ensure HttpListener request buffer is aligned as required by the host processor #25563
Conversation
{ | ||
fixed (byte* pMemoryBlob = memoryBlob) |
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.
The vast majority of the line changes in this file are the result of removing these fixed statements, and thus changing the indentation for large blocks of code.
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.
The vast majority of the line changes in this file are the result of removing these fixed statements, and thus changing the indentation for large blocks of code
FYI, if you append ?w=1 to the GitHub url, it'll ignore whitespace differences:
https://github.com/dotnet/corefx/pull/25563/files?w=1
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.
Good to know!
} | ||
|
||
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.
nit: remove extra tab/spacing.
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.
I'm still seeing this extra space even in your latest set of commits.
protected virtual void Dispose(bool disposing) { } | ||
protected virtual void Dispose(bool disposing) | ||
{ | ||
if (_backingBuffer != IntPtr.Zero) |
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.
Please review IDisposable pattern. Not sure if you should add an 'if' check for "disposing == true" here.
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.
I think that the current behavior is correct, based on this page.
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.
ok. What about a 'finalizer'? Does this have one already and do we need one? Reading the pattern description from your link above says this:
You implement the IDisposable interface and an additional Dispose(Boolean) method, and you also override the Object.Finalize method. You must override Finalize to ensure that unmanaged resources are disposed of if your IDisposable.Dispose implementation is not called by a consumer of your type. If you use the recommended technique discussed in the previous bullet, the System.Runtime.InteropServices.SafeHandle class does this on your behalf.
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.
RequestContextBase does implement a finalizer that calls Dispose(bool), freeing the unmanaged resources it has acquired. I'm not very familiar with the pattern though, so I'll read through the rest of that dispose/finalize article and make sure I'm doing everything correctly.
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.
I've checked it out and am confident the dispose pattern is correctly implemented here.
_backingBufferLength = size; | ||
|
||
// Zero out the contents of the buffer. | ||
for(int i = 0; i < size; ++i) |
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.
nit: add space character after 'for'
@@ -116,7 +125,19 @@ protected void UnsetBlob() | |||
|
|||
protected void SetBuffer(int size) | |||
{ | |||
_backingBuffer = size == 0 ? null : new byte[size]; | |||
if(_backingBuffer != IntPtr.Zero) |
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.
nit: add space character after 'if'
{ | ||
return null; | ||
return RequestBlob; |
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.
spacing looks wrong
for(int i = 0; i < size; ++i) | ||
{ | ||
Marshal.WriteByte(_backingBuffer + i, 0); | ||
} |
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.
How big are these usually? Calling Marshal.WriteByte per byte for a large buffer isn't cheap. Alternatively you could use span:
new Span<byte>((byte*)_backingBuffer, 0, size).Fill(0);
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.
The default size is 4096, but they can be larger than that depending on the size of the request. I really don't like having that loop there, so I'll look into doing it that way instead. Thanks!
@rmkerr Please run outerloop tests as well since this is required for any networking code changes. |
@davidsh HttpListener outerloop tests are passing. |
You need to run the outerloop tests in the PR itself and not just your dev machine. |
Like this? |
Yes, and you should run all platforms affected. And it's probably a good idea to run Linux as well just to be sure that your changes don't impact that platform. |
@dotnet-bot test Outerloop Windows x86 Debug Build please The change should be windows only, but just in case: |
I got failures in the following outerloop tests for HttpClient:
I took a look at the tests, and I don't believe that these failures are related. Especially since the code is only changing HttpListener, which is not used by HttpClient. Just to get more info I'm running Outerloop again. @dotnet-bot test Outerloop Windows x64 Debug Build please |
… processor (dotnet#25563) On Windows, the HttpRecieveHttpRequest function requires a buffer with a memory alignment greater than or equal to the required alignment of the HTTP_REQUEST struct. This fix ensures that alignment requirements are respected when allocating buffers in HttpListener RequestContextBase. Since HttpReceiveHttpRequest copies both the HTTP_REQUEST struct and the variable length request body into the buffer, we need to be able to allocate a buffer with variable size and with a set alignment. Since C# does not provide a method for specifying the alignment of byte arrays, I switched the underlying buffer to be unmanaged. This unmanaged buffer is allocated using Marshal.AllocHGlobal, which allocates memory at the maximum alignment required by the host processor.
…ired by the host processor (#25763) * Ensure HttpListener request buffer is aligned as required by the host processor (#25563) On Windows, the HttpRecieveHttpRequest function requires a buffer with a memory alignment greater than or equal to the required alignment of the HTTP_REQUEST struct. This fix ensures that alignment requirements are respected when allocating buffers in HttpListener RequestContextBase. Since HttpReceiveHttpRequest copies both the HTTP_REQUEST struct and the variable length request body into the buffer, we need to be able to allocate a buffer with variable size and with a set alignment. Since C# does not provide a method for specifying the alignment of byte arrays, I switched the underlying buffer to be unmanaged. This unmanaged buffer is allocated using Marshal.AllocHGlobal, which allocates memory at the maximum alignment required by the host processor. * Remove references to span. * Improve performance when zeroing buffer.
… processor (dotnet/corefx#25563) On Windows, the HttpRecieveHttpRequest function requires a buffer with a memory alignment greater than or equal to the required alignment of the HTTP_REQUEST struct. This fix ensures that alignment requirements are respected when allocating buffers in HttpListener RequestContextBase. Since HttpReceiveHttpRequest copies both the HTTP_REQUEST struct and the variable length request body into the buffer, we need to be able to allocate a buffer with variable size and with a set alignment. Since C# does not provide a method for specifying the alignment of byte arrays, I switched the underlying buffer to be unmanaged. This unmanaged buffer is allocated using Marshal.AllocHGlobal, which allocates memory at the maximum alignment required by the host processor. Commit migrated from dotnet/corefx@357327a
On Windows, the HttpRecieveHttpRequest function requires a buffer with a memory alignment greater than or equal to the required alignment of the HTTP_REQUEST struct.
This fix ensures that alignment requirements are respected when allocating buffers in HttpListener RequestContextBase. Since HttpReceiveHttpRequest copies both the HTTP_REQUEST struct and the variable length request body into the buffer, we need to be able to allocate a buffer with variable size and with a set alignment. Since C# does not provide a method for specifying the alignment of byte arrays, I switched the underlying buffer to be unmanaged. This unmanaged buffer is allocated using Marshal.AllocHGlobal, which allocates memory at the maximum alignment required by the host processor.
I'm new to writing code on the border between managed and unmanaged, and so I would appreciate any suggestions on how to improve things here.
Fixes #25289