DO NOT MERGE: test some refactoring of LoopbackServer #27055

geoffkizer · 2018-02-12T08:07:54Z

No description provided.

stephentoub

Generally looks fine. Thanks.

stephentoub · 2018-02-12T10:53:23Z

src/Common/tests/System/Net/Http/LoopbackServer.cs

-            {
-                try
-                {
-                    s.Shutdown(SocketShutdown.Send);


Why did you remove this shutdown call? It was added specifically to help with test reliability.

Because it shouldn't be necessary. To the extent that it seems to help with test reliability, it appears to mask actual test issues by causing timing delays. This doesn't seem like a good thing.

There was at least one test case where problems cropped up because the server was not sending Content-Length: 0, even though there was no response body. Fixing the test made this problem go away.

Because it shouldn't be necessary

Without it, the client isn't guaranteed to receive all sent data, no?

Without it, the client isn't guaranteed to receive all sent data, no?

Calling Close on the socket does not cause the buffered data to be discarded. The socket will continue to send and retry any buffered data, up to the linger timeout, in the background.

See https://msdn.microsoft.com/en-us/library/windows/desktop/ms738547(v=vs.85).aspx for example, though this isn't the most lucid writeup.

Shutdown is really only useful if you want to do a half-close, i.e. stop sending data but continue to read it. In the HTTP server case, when the server decides to disconnect, it's not interested in any more data from the client, so shutdown is not useful.

At least, that's my understanding.

(That said, shutdown may have local-only effects that are useful, like causing a pending operation to cancel. My point is that in these scenarios, it shouldn't affect what happens on the wire.)

You may very well be right, but there are lots of comments on the web about how this is observably not the case (maybe just due to OS bugs). And from what we've observed, especially in CI, it's also suspect. I suggest don't remove it as part of this refactoring change, as I would not be at all surprised if we saw a resulting increase in spurious test failures. At least submit it as its own dedicated change, maybe a bit after this change guess through and is shown stable.

Linger

Maybe it's the interaction of these that was an issue, with linger timeout set to 0 in some places IIRC.

Ok, I can leave it in.

stephentoub · 2018-02-12T10:54:41Z

src/Common/tests/System/Net/Http/LoopbackServer.cs

+        // CONSIDER: Should this create the HttpClient as well?
+        // Look at usage patterns and see if this makes sense.
+
+        public static Task CreateServerAndClientAsync(Func<Uri, Task> clientFunc, Func<LoopbackServer, Task> serverFunc)


Nit: can you search the order of the parameters, to match the order in the method naming?

I think I'd rather change the method name :)

stephentoub · 2018-02-12T10:57:20Z

src/Common/tests/System/Net/Http/LoopbackServer.cs

+            }
+        }
+
+        // Compatibility methods


Compat with what? This is staging as part of the refactoring?

stephentoub · 2018-02-12T10:59:51Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

                                $"Date: {DateTimeOffset.UtcNow:R}\r\n" +
                                $"Location: {redirectUrl}\r\n" +
+                                "Content-Length: 0\r\n" +
                                "\r\n"));


Is this meant to be part of this change? Seems like the main impact of this is that the connection could be pooled.

Yes, it's intentional; see comment above.

Though, now I'm wondering exactly why we see errors without the Content-Length. I believe these only happened with WinHttpHandler. Maybe I need to understand this better...

davidsh · 2018-02-12T18:09:27Z

I'm confused as to the purpose of this refactoring. Can you explain a little more about the motivation for this? Will it make future tests easier to write? Will it improve stability of the current tests? Thx.

cc: @karelz

geoffkizer · 2018-02-12T18:26:42Z

My intent was mostly to make the test code easier to read and understand, and hopefully to make the usage of LoopbackServer more consistent today and moving forward. I started making a few tweaks and then ended up doing more than I expected.

There are several related changes here, including:
(1) Move nonessential code out of LoopbackServer itself. E.g. the websocket code and the "transfer" code. These are only used by a couple very specific tests, and don't actually need to be in LoopbackServer.
(2) Simplify a few things that are awkward today. For example, the Options object gets passed in to several different places and used in different ways. I changed it to only be passed in to CreateServerAsync. Another example: the callback you pass to AcceptSocketAsync always has to return a Task<List>, even though many uses don't care about this and just pass null.
(3) Add the Connection object, which makes the callback from AcceptSocketAsync cleaner. Today you have to have four params on the callback, even though you often only care about one or two. Connection just encapsulates these args into a single object, and allows you to easily discover and call instance methods on this object.
(4) Rename some core methods to make them more descriptive. "ReadWriteAcceptedAsync" is not entirely obvious.

stephentoub · 2018-02-13T12:42:41Z

Replaced by #27088

Geoff Kizer added 20 commits February 11, 2018 18:28

reorganize and cleanup some LoopbackServer code

023b4ad

add instancing

46d3076

more

2da4e67

more

d65a129

tweaks

9d58d0e

move more stuff

92b11c1

fix WebSockets

e9f00e1

LoopbackConnection WIP with weird WinHttp failures

c3bc97e

weirdness fixed

aa9abbe

cleanup

e90dabf

more

f963c1c

Options cleanup

2bf4df0

more, incl update retry tests

d442b5a

more

ccbce07

more

76461f6

fix a test

2094308

various cleanup

926c8d4

clean up ReadWriteAcceptedAsync

b91f14f

remove ReadWriteAcceptedAsync

e29f7a1

rename ReadRequesAndSendResponseAsync

fb9191f

geoffkizer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 12, 2018

geoffkizer self-assigned this Feb 12, 2018

stephentoub reviewed Feb 12, 2018

View reviewed changes

davidsh added test enhancement Improvements of test source code area-System.Net.Http labels Feb 12, 2018

davidsh mentioned this pull request Feb 12, 2018

Add test infra for auth testing. #26979

Merged

stephentoub closed this Feb 13, 2018

karelz added this to the 2.1.0 milestone Mar 10, 2018

DO NOT MERGE: test some refactoring of LoopbackServer #27055

DO NOT MERGE: test some refactoring of LoopbackServer #27055

Uh oh!

Conversation

geoffkizer commented Feb 12, 2018

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsh commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffkizer commented Feb 12, 2018

Uh oh!

stephentoub commented Feb 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

geoffkizer Feb 12, 2018 •

edited

Loading

davidsh commented Feb 12, 2018 •

edited

Loading