Skip to content

Implement response BodyWriter, CompleteAsync for TestServer #11598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
3 of 4 tasks
Tratcher opened this issue Jun 26, 2019 · 11 comments
Open
3 of 4 tasks

Implement response BodyWriter, CompleteAsync for TestServer #11598

Tratcher opened this issue Jun 26, 2019 · 11 comments
Labels
affected-few This issue impacts only small number of customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Milestone

Comments

@Tratcher
Copy link
Member

Tratcher commented Jun 26, 2019

gRPC depends on pipes and a few other features only implemented by Kestrel such as StartAsync and CompleteAsync. Missing those features in TestServer makes it hard to test gRPC and related components.

  • BodyWriter
  • Reset (HTTP2)
  • StartAsync
  • CompleteAsync

TestServer already uses a pipe internally for its response data, but exposing it turns out to be complicated because we add lots of semantics on top such as the first flush sending headers, content-length verification, etc..

#11305
#11194

@JamesNK

@Tornhoof
Copy link
Contributor

May I ask why it is hard to test, does gRPC not work with the TestServer?

I'm asking for my HttpBatchHandler middleware, which is very similar to the TestServer from ASP.NET Core 2.x, it still works in 3.0 (atleast the current tests do), but I'm not sure if I want to add the Pipe complexity to my middleware, so it would be nice to know which features might rely on new RequestFeature/ResponseFeature functionality (like that IHttpResponseCompletionFeature.CompleteAsync thing).

@JamesNK
Copy link
Member

JamesNK commented Jun 26, 2019

I've just changed gRPC integration tests to use Kestrel instead of test server. HTTP/2 support in HttpClient is rather hot and I wanted more testing with it and gRPC.

grpc/grpc-dotnet#349

These TestServer improves are Good, but they don't need to be a priority because of gRPC.

@Tratcher
Copy link
Member Author

FYI running too many network tests can cause things like socket exhaustion, especially on Mac. Yes we need integration tests with HttpClient, but most unit tests should use TestServer or similar mocks.

@Tratcher
Copy link
Member Author

Tratcher commented Jul 2, 2019

Note to self, Abort was only half implemented. It would fire RequestAborted, but not surface any errors to the client.

@analogrelay
Copy link
Contributor

@Tratcher is this happening as part of your big Response Body Feature thing?

@Tratcher
Copy link
Member Author

Only sortof. All servers (including TestServer) will now have a standard set of APIs (Stream, PipeWriter, StartAsync, CompleteAsync, SendFileAsync), but for this first pass we're polyfilling the implementations. We'll keep this item open fully implement CompleteAsync.

@analogrelay
Copy link
Contributor

Ok, I'll backlog it given that then.

@analogrelay analogrelay added this to the Backlog milestone Jul 24, 2019
@Tratcher Tratcher removed their assignment Jan 22, 2020
@Tratcher Tratcher added affected-few This issue impacts only small number of customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool labels Nov 9, 2020 — with ASP.NET Core Issue Ranking
@kparkov
Copy link

kparkov commented Dec 15, 2020

Is CompleteAsync still missing in .NET 5? I am seeing integration tests fail that worked before, because the exception seems to bubble out of an exception middleware that were supposed to intercept and handle it.

@Tratcher
Copy link
Member Author

@kparkov Yes, CompleteAsync is still a known gap. However, exception handlers should be based on the response starting, not ending. Are you using the built-in exception handler or something else?

@kparkov
Copy link

kparkov commented Dec 30, 2020

I have a custom built exception handler that is added to the end of the middleware chain. Like this:

    public static class GlobalExceptionHandler
    {
        public static void ConfigureGlobalExceptionHandler(this IApplicationBuilder app)
        {
            app.UseExceptionHandler(configure =>
            {
                configure.Use((context, next) =>
                {
                    var exception = context.Features.Get<IExceptionHandlerFeature>()?.Error;

                    if (exception is null)
                    {
                        return next();
                    }
                    else
                    {
                        Feedback result;
                        HttpStatusCode httpStatusCode = HttpStatusCode.InternalServerError;

                        if (exception is ClientException ce)
                        {
                            result = Feedback.Create(code: ce.InternalStatusCode, message: ce.Message, handled: true);
                            httpStatusCode = ce.HttpStatusCode;
                        }
                        else
                        {
#if DEBUG
                            result = Feedback.Create(code: InternalStatusCode.UnhandledServerException, message: exception.Message, handled: false, details: new ExceptionDetails(exception), trace: exception.StackTrace);
#else
                            result = Feedback.Create(code: InternalStatusCode.UnhandledServerException, message: $"An unknown exception of type {exception.GetType().Name} occured.", handled: false);
#endif
                        }

                        var settings = new JsonSerializerSettings();
                        settings.Converters.Add(new StringEnumConverter());

                        var json = JsonConvert.SerializeObject(result, Formatting.Indented, settings);

                        context.Response.StatusCode = (int) httpStatusCode;
                        return context.Response.WriteAsync(json);
                    }
                });
            });
        }
    }

However, the test server throws an HttpRequestException exception, even though any exception is caught here. This was not the behaviour before .NET 5, and I don't really understand why.

EDIT: I considered modifying it using CompleteAsync to make sure it was completed normally (not in an error state), but couldn't find the method available, hence why I posted here.

@Tratcher
Copy link
Member Author

That looks fine. UseExceptionHandler shouldn't re-throw exceptions after you've called Response.WriteAsync. However, it will log and rethrow the original exception if your exception handler throws. Have you checked the logs and/or stepped through your exception handler in the debugger?

@amcasey amcasey added area-hosting Includes Hosting and removed feature-hosting labels Jun 1, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

8 participants