Initial commit to System.Net.HttpListener #13502

Priya91 · 2016-11-08T23:13:49Z

This gets @ericeil PR #13287 in. Fixed merged conflicts.

cc @stephentoub @davidsh @CIPop @geoffkizer @karelz

stephentoub · 2016-11-09T15:14:45Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+{
+    internal unsafe static partial class HttpApi
+    {
+        internal static readonly HTTPAPI_VERSION Version = new HTTPAPI_VERSION() { HttpApiMajorVersion = 2, HttpApiMinorVersion = 0 };


Nit: s_version?

stephentoub · 2016-11-09T15:15:01Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+    {
+        internal static readonly HTTPAPI_VERSION Version = new HTTPAPI_VERSION() { HttpApiMajorVersion = 2, HttpApiMinorVersion = 0 };
+
+        internal static readonly bool Supported = InitHttpApi(Version);


Nit: s_supported?

stephentoub · 2016-11-09T15:16:46Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+
+internal static partial class Interop
+{
+    internal unsafe static partial class HttpApi


Can we scope the unsafe to be only where it's needed, rather than the whole class?

stephentoub · 2016-11-09T15:19:21Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+        internal struct HTTP_KNOWN_HEADER
+        {
+            internal ushort RawValueLength;
+            internal sbyte* pRawValue;


Why sbyte rather than byte? (Same question for the other occurrences below.)

Same as desktop code.

stephentoub · 2016-11-09T15:21:24Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+        [DllImport(Libraries.HttpApi, SetLastError = true)]
+        internal static extern uint HttpCloseServerSession(ulong serverSessionId);
+
+        internal class SafeLocalFreeChannelBinding : ChannelBinding


Nit: sealed?

stephentoub · 2016-11-09T15:21:58Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+            {
+                SafeLocalFreeChannelBinding result;
+
+                result = LocalAllocChannelBinding(LMEM_FIXED, (UIntPtr)cb);


Nit: the declaration and initialization can be combined

stephentoub · 2016-11-09T15:23:28Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+        internal class SafeLocalFreeChannelBinding : ChannelBinding
+        {
+            private const int LMEM_FIXED = 0;
+            private int _size;


This type needs a parameterless ctor in order to work with the P/Invoke. The compiler is providing one by default, but it's public... consider adding an explicit one that's private.

stephentoub · 2016-11-09T15:24:15Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+        }
+
+        [DllImport(Interop.Libraries.Heap, EntryPoint = "LocalAlloc", SetLastError = true)]
+        internal static extern SafeLocalFreeChannelBinding LocalAllocChannelBinding(int uFlags, UIntPtr sizetdwBytes);


Nit: Why not just call the method LocalAlloc rather than calling it LocalAllocChannelBinding and using EntryPoint to change its name?

Yeah makes sense, it was copied from Desktop code.

stephentoub · 2016-11-09T15:24:39Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+                "WWW-Authenticate",
+            };
+
+            private static Dictionary<string, int> s_hashtable = CreateTable();


Nit: readonly

stephentoub · 2016-11-09T15:26:44Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+            //}
+
+            //NetEventSource.Exit(null);
+            //return endpoint;


What's the plan for all of this commented out code? Is there a TODO to follow-up on this?

Yeah, am working on it.

stephentoub · 2016-11-09T15:26:54Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+            //}
+
+            //NetEventSource.Exit(null);
+            //return endpoint;


Same comment/question

stephentoub · 2016-11-09T15:29:17Z

src/Common/src/Interop/Windows/sspicli/SafeDeleteContext.cs

+
+namespace System.Net.Security
+{
+    //


How does this SafeDeleteContext differ from the one we already have in the repo?
https://github.com/dotnet/corefx/blob/master/src/Common/src/System/Net/Security/Unix/SafeDeleteContext.cs

Oh, I see, you just separated it out of SecuritySafeHandles.cs. Ok.

In reply to: 87213631 [](ancestors = 87213631)

stephentoub · 2016-11-09T15:30:25Z

src/Common/src/System/Net/DebugSafeHandle.cs

@@ -43,7 +43,6 @@ private void Trace()

        ~DebugSafeHandle()
        {
-            DebugThreadTracking.SetThreadSource(ThreadKinds.Finalization);


Why was this removed? I don't know what its purpose was, just wondering why it's going away.

cc @ericeil

stephentoub · 2016-11-09T15:33:15Z

src/System.Net.HttpListener/src/System/Net/AsyncRequestContext.cs

+
+namespace System.Net
+{
+    internal unsafe class AsyncRequestContext : RequestContextBase


Nit: sealed?

stephentoub · 2016-11-09T15:33:34Z

src/System.Net.HttpListener/src/System/Net/AsyncRequestContext.cs

+    {
+        private NativeOverlapped* _nativeOverlapped;
+        private ThreadPoolBoundHandle _boundHandle;
+        private ListenerAsyncResult _result;


Nit: readonly

stephentoub · 2016-11-09T15:34:38Z

src/System.Net.HttpListener/src/System/Net/AsyncRequestContext.cs

+            if (_nativeOverlapped != null)
+            {
+                Debug.Assert(!disposing, "AsyncRequestContext::Dispose()|Must call ReleasePins() before calling Dispose().");
+                if (!Environment.HasShutdownStarted || disposing)


!Environment.HasShutdownStarted || [](start = 20, length = 35)

Do we know why this check is here?

@CIPop Any idea?

stephentoub · 2016-11-09T15:37:04Z

src/System.Net.HttpListener/src/System/Net/CaseInsensitiveAsciiComparer.cs

+
+namespace System.Net
+{
+    internal class CaseInsensitiveAsciiComparer : IEqualityComparer, IComparer


How does this differ from the CaseInsensitiveAscii comparer already in the repo?
https://github.com/dotnet/corefx/blob/master/src/System.Net.WebHeaderCollection/src/System/Net/WebHeaderCollection.cs#L496
If possible, we should combine them to avoid duplication.

Yeah, confirmed it's the same. Will reuse.

stephentoub · 2016-11-09T15:37:50Z

src/System.Net.HttpListener/src/System/Net/CookieExtensions.cs

+                s_toServerStringFunc = (Func<Cookie, string>)typeof(Cookie).GetMethod("ToServerString").CreateDelegate(typeof(Func<Cookie, string>));
+            }
+            return s_toServerStringFunc(cookie);
+        }


@weshaggard, FYI on another reflection case to access internals in another assembly.

Shouldn't we add a TODO here?
This can be solved in other ways (e.g. factoring then importing the relevant code or exposing a public API, etc.)

Created issue #13607 to track.

stephentoub · 2016-11-09T15:38:29Z

src/System.Net.HttpListener/src/System/Net/HttpListener.cs

+            //}
+            finally
+            {
+                //if (NetEventSource.IsEnabled) NetEventSource.Exit(NetEventSource.ComponentType.HttpListener, this, "Close", "");


All of this tracing should be fixed and uncommented.

stephentoub · 2016-11-09T15:39:14Z

src/System.Net.HttpListener/src/System/Net/HttpListener.Unix.cs

+namespace System.Net
+{
+    public sealed unsafe partial class HttpListener
+    {


Please add a TODO comment for https://github.com/dotnet/corefx/issues/13187

stephentoub · 2016-11-09T15:40:52Z

src/Common/src/Interop/Windows/HttpApi/Interop.HttpApi.cs

+
+        private static bool InitHttpApi(HTTPAPI_VERSION version)
+        {
+            uint statusCode = HttpInitialize(version, (uint)HTTP_FLAGS.HTTP_INITIALIZE_SERVER, null);


Are there any platforms we need to run on where this function isn't exported? I'm wondering if we need a try/catch here to return false if this fails for any reason.

In .Net Desktop we check for Win7+ for this call.

@Priya91 To clarify: this is good as written and no more changes are required. CoreFX will only run on Win7 and above which support the above initialization code.

stephentoub · 2016-11-09T15:42:09Z

src/System.Net.HttpListener/src/System/Net/HttpListener.Windows.cs

+        private string _realm;
+        private SafeHandle _requestQueueHandle;
+        private ThreadPoolBoundHandle _requestQueueBoundHandle;
+        private volatile State _state; // m_State is set only within lock blocks, but often read outside locks. 


Nit: wrong name

stephentoub · 2016-11-09T15:42:41Z

src/System.Net.HttpListener/src/System/Net/HttpListener.Windows.cs

+        private HttpListenerTimeoutManager _timeoutManager;
+        private bool _V2Initialized;
+
+        private Hashtable _disconnectResults;         // ulong -> DisconnectAsyncResult


Hashtable _disconnectResults; // ulong -> DisconnectAsyncResult [](start = 16, length = 71)

Should this be a Dictionary<ulong, DisconnectAsyncResult> then?

stephentoub · 2016-11-09T15:43:11Z

src/System.Net.HttpListener/src/System/Net/HttpListener.Windows.cs

+        private Hashtable _disconnectResults;         // ulong -> DisconnectAsyncResult
+        private object _internalLock;
+
+        internal Hashtable m_UriPrefixes = new Hashtable();


m_UriPrefixes [](start = 27, length = 13)

Nit: _uriPrefixes

stephentoub · 2016-11-09T15:43:21Z

src/System.Net.HttpListener/src/System/Net/HttpListener.Windows.cs

+        private bool _V2Initialized;
+
+        private Hashtable _disconnectResults;         // ulong -> DisconnectAsyncResult
+        private object _internalLock;


stephentoub · 2016-11-09T15:43:49Z

src/System.Net.HttpListener/src/System/Net/HttpListener.Windows.cs

+                CheckDisposed();
+                if (value == null)
+                {
+                    throw new ArgumentNullException();


nameof(value)?

clrjunkie · 2016-11-11T22:41:00Z

Big Plus for working on this!
Big Minus for "viewing this work as supporting mostly non-production web server scenarios."

HttpListener must be treated as a key infrastructure component for productions systems from day one.

It only stands to reason that this would ultimately be the underlying http server for ASP.Net Core (as in the case of Full FX ASP.NET self hosting ) and "Kestrel" would be a thing of the past...

stephentoub · 2016-11-11T22:49:50Z

HttpListener must be treated as a key infrastructure component for productions systems from day one. It only stands to reason that this would ultimately be the underlying http server for ASP.Net Core (as in the case of Full FX ASP.NET self hosting ) and "Kestrel" would be a thing of the past

Thank you for the feedback, but no. The ASP.NET team (both at Microsoft and the community) has and continues to invest years and years of dev time into making that stack as efficient and programmable as possible, on both Windows and Unix, evolving it both in terms of implementation and API to do so. We do not plan to duplicate such efforts with HttpListener, nor are we able to take the API-level breaks necessary to bring it up to performance par. It is being exposed as part of corefx for compatibility, to enable code currently using it, but for production scenarios, I recommend using kestrel, ASP.NET, etc.

clrjunkie · 2016-11-11T23:10:10Z

@stephentoub The "ASP.NET" Stack has nothing to do with either HttpListener or "Kestrel" and if backward compatibility drives this effort then your duplicating efforts (and wasting time) as we speak... since nobody should care about the underlying HttpListener implementation (e.g Http.sys) but rather about the interface so for all intent and purposes "Kestrel" should expose HttpListener interface and get it over with. It's a simple interface and there is no reason it should not suit production scenarios regardless of the stack built on top of it.

stephentoub · 2016-11-11T23:27:16Z

That is not the direction we are going. Thanks.

…w one.

Priya91 · 2016-11-15T01:23:04Z

@stephentoub @CIPop Addressed the PR feedback and filed issues on pending workitems. Plan to merge PR after CI passes, will continue watching the PR, and address additional comments in separate PR. This PR is getting too big to keep track of the work.

cc @karelz

stephentoub · 2016-11-15T01:38:56Z

Thanks, @Priya91.

dnfclas added the cla-already-signed label Nov 8, 2016

davidsh added area-System.Net netstandard2.0 labels Nov 8, 2016

karelz assigned Priya91 Nov 9, 2016

stephentoub reviewed Nov 9, 2016

View reviewed changes

danmoseley modified the milestone: 1.2.0 Nov 11, 2016

Eric Eilebrecht and others added 15 commits November 14, 2016 16:43

Add System.Net.HttpListener

74b8ef3

Disable logging. Undo this commit before fixing the logging code.

ff8b9e0

More work on HttpListener

aa293e3

Rename contract: System.Net.Http.Listener->System.Net.HttpListener

adfbcd3

Use existing "common" HttpKnownHeaderNames.cs rather than adding a ne…

ba59348

…w one.

Address PR feedback

84b7f8d

More PR feedback

e07003f

Fix conflicts in project.json dependency.

b5f288f

Fix issue on using internal SocketAddress in HttpListener.

e24a00f

Add NetEvenSource logging to System.Net.HttpListener.

c612a92

Use nameof operator in exception arguments.

183a013

Move CaseInsensitiveAscii type to Common location.

e8ebf51

Address nits and other changes from PR feedback.

3f1f8b3

Move WebSocket Interop to Common Interop location.

2c6f7fd

Update version on dependencies.

b70e653

Priya91 force-pushed the HttpListener branch from dab4478 to b70e653 Compare November 15, 2016 01:13

Add Newlines at EoF.

71ca4dc

Priya91 merged commit 561050c into dotnet:master Nov 15, 2016

Priya91 deleted the HttpListener branch November 15, 2016 03:28

chrisdunelm mentioned this pull request Feb 28, 2017

Calendar API missing packages (GoogleWebAuthorizationBroker and FileDataStore) for .net core googleapis/google-api-dotnet-client#897

Closed

clrjunkie mentioned this pull request Jan 31, 2020

Reconsider port to core approach for System.Net.HttpListener dotnet/runtime#19350

Closed

Priya91 mentioned this pull request Jan 31, 2020

Reuse ManagedWebSocket for websocket connections on Windows HttpListener. dotnet/runtime#19367

Closed

Initial commit to System.Net.HttpListener #13502

Initial commit to System.Net.HttpListener #13502

Conversation

Priya91 commented Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clrjunkie commented Nov 11, 2016

stephentoub commented Nov 11, 2016

clrjunkie commented Nov 11, 2016

stephentoub commented Nov 11, 2016

Priya91 commented Nov 15, 2016 • edited Loading

stephentoub commented Nov 15, 2016

Priya91 commented Nov 8, 2016 •

edited

Loading

stephentoub Nov 9, 2016 •

edited

Loading

Priya91 commented Nov 15, 2016 •

edited

Loading