P/Invoke cleanup #60

Tragetaschen · 2015-02-08T15:44:06Z

This PR cleans up large portions of the P/Invoke handling of Kestrel to libuv. I know these are a lot of changes, but I have kept the single commits for easier following.

The highlights are:

~~Use DllImport and Mono's DllMap feature instead of manually loading and binding a platform-specific library~~
The various SafeHandles now do all the required allocation work in their constructors
No more unsafe
Working shutdown

I have tested on both Windows and Linux, but could not do so on a Mac simply because I don't have access to one. My test setup included a web application with an active SignalR connection. I was not able to reproduce any of the shutdown issues again (yet?)

ghost · 2015-02-08T15:44:10Z

Hi @Tragetaschen, I'm your friendly neighborhood Microsoft Open Technologies, Inc. Pull Request Bot (You can call me MSOTBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft Open Technologies, Inc. and real humans are currently evaluating your PR.

TTYL, MSOTBOT;

lodejard · 2015-02-08T21:02:37Z

Wow, that's a lot of work! The individual commits are very helpful and they sound like good ideas based on the descriptions. We'll take a closer look soon. Thank you very much!

Tragetaschen · 2015-02-09T14:54:09Z

As a final step (well, let's see about that):

UvHandles are not SafeHandles anymore

Since they have to clean both loop resource as well as the unmanaged memory, the SafeHandle pattern does not really work: By the time the Finalizer runs, the loop might have been long brought down and posting to it won't work. The new base class implements a working UvHandle pattern and warns when a finalizer does the cleanup (I don't get the warning).

Tragetaschen · 2015-02-09T17:20:08Z

Indirectly, I know that OSx won't work, but it's close. I have modified my Linux environment so it looks similar to how OSx will behave (basically removed libuv* from /usr/lib64 and placed the files in various folders).

The reason it worked on Linux so far is that mono is quite smart with DllImports. "libuv.dll" is mapped to "libuv.so" and then searched for in the standard folders. Because I have a link libuv.so -> libuv.so.1.0.0 in /usr/lib64, everything Just Works (tm). On OSx, a compatible file is not in the standard folders, though.

I'll revert to the manual binding :-/

Eilon · 2015-02-09T18:23:14Z

src/Microsoft.AspNet.Server.Kestrel/Networking/Libuv.cs

            {
-                IsDarwin = PlatformApis.IsDarwin();
+#if ASPNETCORE50
+                return true;


While this is true now, it won't be true in the future... what would be the correct code once Core CLR runs on non-Windows?

Good question.

This code is the same as before, I just took it as is. However, I spent a few thoughts and decided I don't know enough to fix it.

Ah OK we'll figure this out in the future, then.

davidfowl · 2015-02-09T19:38:15Z

src/Microsoft.AspNet.Server.Kestrel/Http/SocketOutput.cs

            private static void WriteCallback(UvWriteReq req, int status, Exception error, object state)
            {
                ((ThisWriteReq)state).OnWrite(req, status, error);
            }

            SocketOutput _self;
-            ArraySegment<byte> _buffer;
+            byte[] _buffer;


The only code path into the Contextualize currently creates a copy of the incoming ArraySegment. I figured I can simplify the code down to the P/Invoke.

That part of the server hasn't been optimized at all really (which is why it's copying the array segment every time) - so I wouldn't worry too much about fixing that code path before it's been fine-tuned.

Tragetaschen · 2015-02-09T22:06:36Z

No more manual writing in unmanaged memory
Clean shutdown logic (no more uv_stop)

Tragetaschen · 2015-02-10T15:25:15Z

I will now stop shuffeling code around :)

Eilon · 2015-02-20T18:44:00Z

@lodejard can you try this out locally and merge?

Tragetaschen · 2015-02-20T18:46:40Z

I'm close to fixing most of the bugs I found so far (see the -beta2 branch in my fork). For stability reasons, I'd prefer that additional work to be included

Tragetaschen · 2015-02-20T19:52:53Z

Connections are now closed cleanly on shutdown as opposed to through the Finalizer
Keep-Alive working
Connections closed by the client working

I'm not quite convinced of the last commit. Not that I couldn't defend its practical correctness, but some code paths down the MessageBody.Consume implementations are not that clean in their handling of RemoteIntakeFin and Buffer.Count == 0, and these aren't triggered anymore through the Frame.

The leaking SignalR connections currently trigger a Console warning during shutdown.

Tragetaschen · 2015-02-20T21:17:59Z

I've done a fair amount of testing including ab stress testing with half a million requests and I don't expect to have introduced any regressions while fixing a couple of issues.

Tragetaschen · 2015-02-21T09:01:25Z

There was a race between closing the socket client-wise on the one hand and still writing from the application's end. The write request could throw an ObjectDisposedException on its on-loop-part that would bring down the loop.

Instead of catching this exception, I changed the Write to WriteAsync. Now, the exception still occurs, but is handled by whomever is currently handling the request.

Tragetaschen · 2015-02-23T16:57:53Z

When a connection with an active write request is closed, the write request's callback gets -UV_ECANCELED as error code. This is now handled as an expected error.

aspnet#62

aspnet#61

…writing resulting in an ObjectDisposedException that would bring down the loop

handling for well-formed requests.

Tragetaschen · 2015-03-14T15:03:08Z

Sure

Tratcher · 2015-04-07T21:06:29Z

src/Microsoft.AspNet.Server.Kestrel/Http/Connection.cs

            {
                KestrelTrace.Log.ConnectionReadFin(_connectionId);
                SocketInput.RemoteIntakeFin = true;
-                _socket.ReadStop();
+                if (status != -4095)


Can you define a descriptive constant for this? The same goes for the ones above.

dnfclas · 2015-05-07T17:11:44Z

@Tragetaschen, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

Tragetaschen · 2015-07-21T13:29:10Z

Watching last week's Community Standup made me really, really sad :(

ghost added the cla-not-required label Feb 8, 2015

Tragetaschen force-pushed the pinvoke-cleanup branch from 0fb3e9b to c93c029 Compare February 9, 2015 14:43

Eilon reviewed Feb 9, 2015
View reviewed changes

Tragetaschen force-pushed the pinvoke-cleanup branch 3 times, most recently from 3b1443e to 1ee6990 Compare February 9, 2015 19:35

davidfowl reviewed Feb 9, 2015
View reviewed changes

Tragetaschen force-pushed the pinvoke-cleanup branch from 846221c to 867582f Compare February 11, 2015 08:52

Eilon assigned lodejard Feb 20, 2015

Tragetaschen force-pushed the pinvoke-cleanup branch from d5baac8 to b83615f Compare February 20, 2015 19:45

Tragetaschen mentioned this pull request Feb 20, 2015

Shutting the server down causes a hang #9

Closed

Tragetaschen force-pushed the pinvoke-cleanup branch from d5cc00e to b449d77 Compare February 21, 2015 09:10

Kai Ruhnau added 17 commits March 14, 2015 15:11

Disposed read handles when closing the connection

1560adc

Registered connections on the listener to close them during shutdown.

603b9ea

aspnet#62

Improved the object validation

9602fd1

Dispose the socket when the client closes the connection

7ce00d0

aspnet#61

Early-exit when the connection is closed

74a04c2

Write is now Async. This fixes a race between closing the socket and …

7245a17

…writing resulting in an ObjectDisposedException that would bring down the loop

Removed the write callback function

7e34f49

Active write requests are canceled when the socket is closed

2ba41b7

Removed .Net 4.6'isms

37e8715

Code style

b43b306

Propagate the read status code and add proper EOF

a93cc80

handling for well-formed requests.

Used different ports in the test classes

5ea8cc0

Added API to check for active connections

0b7b166

Moved a failing test to an issue class

ebf3c5f

Added a few ways to create a leaked connection

65e87b7

Provide Write errors to upper layers

3f9628f

Activated the (reliable!) engine tests

05e6404

Tragetaschen force-pushed the pinvoke-cleanup branch from 73c3fb8 to 05e6404 Compare March 14, 2015 14:12

Tratcher reviewed Apr 7, 2015
View reviewed changes

Tragetaschen mentioned this pull request Apr 30, 2015

There is no libdl on FreeBSD #94

Closed

dnfclas added the cla-signed label May 7, 2015

Tragetaschen mentioned this pull request Jun 14, 2015

Server deadlock #103

Closed

dougbu mentioned this pull request Jun 23, 2015

Regression on stopping the server #111

Closed

Tragetaschen mentioned this pull request Jun 24, 2015

Move non-nested classes and interfaces to their own files #112

Closed

Tragetaschen mentioned this pull request Jul 5, 2015

Create an unsafe native methods type #122

Closed

Tragetaschen closed this Jul 8, 2015

yuwaMSFT mentioned this pull request Feb 25, 2016

AV in coreclr during long haul with Ngnix #656

Closed

P/Invoke cleanup #60

P/Invoke cleanup #60

Uh oh!

Conversation

Tragetaschen commented Feb 8, 2015

Uh oh!

ghost commented Feb 8, 2015

Uh oh!

lodejard commented Feb 8, 2015

Uh oh!

Tragetaschen commented Feb 9, 2015

Uh oh!

Tragetaschen commented Feb 9, 2015

Uh oh!

Eilon Feb 9, 2015

Choose a reason for hiding this comment

Uh oh!

Tragetaschen Feb 9, 2015

Choose a reason for hiding this comment

Uh oh!

Eilon Feb 9, 2015

Choose a reason for hiding this comment

Uh oh!

davidfowl Feb 9, 2015

Choose a reason for hiding this comment

Uh oh!

Tragetaschen Feb 9, 2015

Choose a reason for hiding this comment

Uh oh!

lodejard Feb 9, 2015

Choose a reason for hiding this comment

Uh oh!

Tragetaschen commented Feb 9, 2015

Uh oh!

Tragetaschen commented Feb 10, 2015

Uh oh!

Eilon commented Feb 20, 2015

Uh oh!

Tragetaschen commented Feb 20, 2015

Uh oh!

Tragetaschen commented Feb 20, 2015

Uh oh!

Tragetaschen commented Feb 20, 2015

Uh oh!

Tragetaschen commented Feb 21, 2015

Uh oh!

Tragetaschen commented Feb 23, 2015

Uh oh!

Tragetaschen commented Mar 14, 2015

Uh oh!

Tratcher Apr 7, 2015

Choose a reason for hiding this comment

Uh oh!

dnfclas commented May 7, 2015

Uh oh!

Tragetaschen commented Jul 21, 2015

Uh oh!

Uh oh!