Skip to content

Implement MaxRequestBodySize feature for IIS inprocess #9475

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

Merged
merged 17 commits into from
May 6, 2019

Conversation

jkotalik
Copy link
Contributor

Fixes #8293.

I'm okay if this slips to preview6. I'll leave comments in the diff.

@jkotalik jkotalik added this to the 3.0.0-preview5 milestone Apr 17, 2019
@jkotalik jkotalik requested a review from analogrelay as a code owner April 17, 2019 21:19
@analogrelay
Copy link
Contributor

Moving this out of preview 5. We are starting to limit preview 5 work to essential stuff only.

@jkotalik
Copy link
Contributor Author

I'm going to block myself on merging this until I figure out why a set of IIS tests aren't running on PRs.

@jkotalik jkotalik closed this Apr 24, 2019
@jkotalik jkotalik reopened this Apr 24, 2019
@jkotalik
Copy link
Contributor Author

Re-enabled some IIS tests, this should now report success/failures correctly now.

@jkotalik jkotalik force-pushed the jkotalik/bodySizeFeature branch from ee61480 to e59736e Compare April 29, 2019 16:54
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 also wonder if we can integrate closer with maxAllowedContentLength, or even override it? Let's talk offline.

@jkotalik jkotalik force-pushed the jkotalik/bodySizeFeature branch from 94eefa3 to 56f0de6 Compare May 3, 2019 01:15
@jkotalik jkotalik requested a review from Tratcher May 3, 2019 01:17
@jkotalik
Copy link
Contributor Author

jkotalik commented May 3, 2019

This now adds support for reading the IIS content length limit and logs a warning per request/in the constructor for the Server.

@jkotalik
Copy link
Contributor Author

jkotalik commented May 6, 2019

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2479

@jkotalik jkotalik merged commit aaaaf57 into master May 6, 2019
@jkotalik jkotalik deleted the jkotalik/bodySizeFeature branch May 6, 2019 02:30
@discostu105
Copy link

discostu105 commented Jun 14, 2019

We just upgraded to preview-6 and suddenly our web.config setting (maxAllowedContentLength) seems to not be working anymore. We run on IIS in-process. Here is the config: https://github.com/Dynatrace/superdump/blob/master/src/SuperDumpService/web.config

Anything wrong with the config at first glance? Should we file an issue for this? @jkotalik

@Tratcher
Copy link
Member

seems to not be working anymore

What is the error?

@discostu105
Copy link

the setting from web.config is not respected anymore, so it uses a much lower default.

@analogrelay
Copy link
Contributor

analogrelay commented Jun 17, 2019

If you're seeing the setting be ignored, definitely file a new bug and provide a runnable sample that reproduces the problem. We can always close it if it turns out there isn't a problem (though if the behavior is confusing to you, that's a problem ;)). We don't track merged PRs like this so it's likely this will get lost if we stop checking our GitHub notifications ;). Also, include any logs from the app since it looks like we should be logging a warning if there's a conflict between these options.

@jkotalik From a very cursory look at the code, I wonder if the fact that we set a default size limit in the managed code is conflicting here. Perhaps the default (if the setting isn't set) should be to just use whatever the web.config value is (if one is present)?

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

This server does not support the IHttpRequestBodySizeFeature
7 participants