Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Added feature to control http body behavior #821

Merged
merged 2 commits into from
Apr 22, 2017
Merged

Conversation

davidfowl
Copy link
Member

  • Added a flag to allow synchronous IO

- Added a flag to allow synchronous IO
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting a global flag on KestrelOptions, not a per request flag. Or do you plan to do both?

@JunTaoLuo
Copy link
Contributor

I assume there is more to this PR than just this?

@davidfowl
Copy link
Member Author

I was expecting a global flag on KestrelOptions, not a per request flag. Or do you plan to do both?

Both.

I assume there is more to this PR than just this?

This is just adding the feature. I don't plan to push all at once. Feature first, then Kestrel change once it goes in.

@@ -0,0 +1,17 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright

@@ -0,0 +1,17 @@
using System;
using System.Collections.Generic;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you using from these namespaces?

/// <summary>
/// Controls the IO behavior for the <see cref="IHttpRequestFeature.Body"/> and <see cref="IHttpResponseFeature.Body"/>
/// </summary>
public interface IHttpBodyControlFeature
Copy link
Contributor

@JunTaoLuo JunTaoLuo Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the naming BodyControl. Maybe IHttpBodyIOOptionsFeature, IHttpBodyIOFeature, IHttpBodyIOCapabilitiesFeature?

@halter73
Copy link
Member

@Tratcher Is HttpSysServer going to the same global flag?

It would be pretty simple to disallow sync IO using some middleware to wrap the request and response body streams. The perf hit won't be too bad if the wrapping stream is pooled and lazily initialized. It would also ensure that a server switch doesn't suddenly cause a bunch of Read/Write calls to fail.

@JunTaoLuo
Copy link
Contributor

Still don't like the naming, but functionally it's fine.

@davidfowl
Copy link
Member Author

@DamianEdwards do you have any naming suggestions?

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 yes, HttpSys could add the same global and feature flags, but there's no bug filed for it. SyncIO is just as bad for HttpSys because you can block in native and can't cleanly cancel.

@DamianEdwards
Copy link
Member

IHttpBodyIOControlFeature
IHttpIOControlFeature
IHttpBodyIOFeature
IHttpBodySyncIOControlFeature
IHttpRequestResponseBodySynchronousIOControlFeature
IHttpIOIOIOItsOffToWorkWeGoFeature

@davidfowl davidfowl merged commit 5472763 into dev Apr 22, 2017
@smitpatel smitpatel deleted the davidfowl/synchronous-io branch June 28, 2017 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants