Bug 5598 - Regression: Memory leak in asp.net (low level) for each unique URL request.
Summary: Regression: Memory leak in asp.net (low level) for each unique URL request.
Status: RESOLVED FIXED
Alias: None
Product: Class Libraries
Classification: Mono
Component: System.Web ()
Version: 2.10.x
Hardware: PC Windows
: --- normal
Target Milestone: Untriaged
Assignee: Marek Habersack
URL:
Depends on:
Blocks:
 
Reported: 2012-06-10 03:09 UTC by Bassam
Modified: 2013-05-27 16:36 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
stopgap (534 bytes, patch)
2012-09-26 13:23 UTC, michael
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Bassam 2012-06-10 03:09:36 UTC
The problem reported here https://bugzilla.novell.com/show_bug.cgi?id=443569 has resurfaced as of https://github.com/mono/mono/commit/692b3e376b2993b0ea0f9a87692ca221dacc47c4. 

I have verified that WebConfigurationManager.GetSection is adding an entry to the section cache for each unique URL path. For example for '/File/foo/bar/hello' URL the following is logged:

WebConfigurationManager.GetSection: Lookup by configPath '/' succeeded for section 'system.web/globalization
WebConfigurationManager.GetSection: Lookup by path '/' succeeded for section 'system.web/machineKey
WebConfigurationManager.GetSection: Adding to section cache by cachePath '/File/foo/bar/hello'
WebConfigurationManager.GetSection: Lookup by path '/File/foo/bar/hello' succeeded for section 'system.web/customErrors

note that an entry was added by cachePath '/File/foo/bar/hello' even though I have a single application with vdir / and no nested web.config, or location sections.

We found this because after millions of paths we encountered a hash collision where two paths hash to the same value, and the wrong section is loaded from the section cache, leading to the following:


Cannot cast from source type to destination type.
Description: HTTP 400. Error processing request.
Stack Trace:
System.InvalidCastException: Cannot cast from source type to destination type.
  at System.Web.HttpApplication.PreStart () [0x00000] in <filename unknown>:0 
  at System.Web.HttpApplication.Start (System.Object x) [0x00000] in <filename unknown>:0 
  at System.Web.HttpApplication.System.Web.IHttpHandler.ProcessRequest (System.Web.HttpContext context) [0x00000] in <filename unknown>:0 
  at System.Web.HttpRuntime.Process (System.Web.HttpWorkerRequest req) [0x00000] in <filename unknown>:0 
  at System.Web.HttpRuntime.RealProcessRequest (System.Object o) [0x00000] in <filename unknown>:0 
  at System.Web.HttpRuntime.ProcessRequest (System.Web.HttpWorkerRequest wr) [0x00000] in <filename unknown>:0 
  at Mono.WebServer.MonoWorkerRequest.ProcessRequest () [0x00000] in <filename unknown>:0 

Thanks!
Comment 1 Miguel de Icaza [MSFT] 2012-08-15 16:53:08 UTC
grendel, this is a regression in some recent chagnes.
Comment 2 michael 2012-09-26 13:23:18 UTC
Created attachment 2624 [details]
stopgap

This is a possible fix/stopgap.
Comment 3 Miguel de Icaza [MSFT] 2012-09-27 16:39:11 UTC
grendel  ping
Comment 4 Marek Habersack 2012-09-27 17:00:58 UTC
It's not really a regression. This entire code was written to work around problems with the horrible thing System.Configuration is. Removing caching will slow things down drastically. The proposed fix isn't acceptable either, alas, as it would basically remove the optimization. What would be acceptable, however, is to clean the section cache after it exceeds a certain number of entries and/or after certain time passes.
Comment 5 Bassam 2012-09-27 17:32:29 UTC
An alternative is to expose an option to turn off caching. For MVC/RESTful applications, the URL caching does not add much value as almost every URL is different.
Comment 6 Miguel de Icaza [MSFT] 2012-09-27 17:55:39 UTC
What about using an LRU cache?

Here is a small one I wrote that can be adapted:

https://github.com/migueldeicaza/MonoTouch.Dialog/blob/master/MonoTouch.Dialog/Utilities/LRUCache.cs
Comment 7 Marek Habersack 2012-09-27 19:11:10 UTC
We have an LRU cache in System.Web as well, used for System.Web.Caching, it could be tailored for this purpose. Or we could use Miguel's implementation. The problem is the time - I don't think I'll have time to implement it until November/December at least.

Bassam, this code still benefits MVC because on each request, whether for a virtual or physical location, we must check whether Web.config exists at the location. This is to make sure that configuration items have the values from the directory closes to the current request. If you disable the cache, then the check will require a trip to the virtual directory/file providers and, in most cases, will entail filesystem access and, as a result, will slow your requests down. This whole implementation is suboptimal, but we were working around shortcomings of System.Configuration in general and our implementation of it in particular. Some form of caching must be present, so perhaps it'd be best to use the LRU cache. Patches accepted :)
Comment 8 Andres G. Aragoneses 2013-04-23 11:53:26 UTC
(In reply to comment #7)
> Patches accepted :)

I've created a pull request for this (since we were also affected):

https://github.com/mono/mono/pull/618

> We have an LRU cache in System.Web as well, used for System.Web.Caching, it
> could be tailored for this purpose. Or we could use Miguel's implementation.

I've looked at the current LRU cache, which I was a bit hesitant to use because it doesn't use generic types* and because it has some expiration logic which we don't need in this case I think. I also tweaked a bit Miguel's LRU to have same API as the dictionary used in WebConfigurationManager (basically Add() and TryGetValue() instead of the indexer), and to remove the unneeded size calculations (as there should be enough with just a limit of entries, no?).

I've used a MAX limit of 100 section, but let me know if this is too low.
Thanks
Comment 9 Andres G. Aragoneses 2013-04-23 11:54:14 UTC
* = yes I know the current usage is "object" type for the value, but at least a generic type is used for the key...
Comment 10 Miguel de Icaza [MSFT] 2013-04-23 12:03:56 UTC
I love Andres' approach (and not only because he used my LRU cache).

I have one request: we do not know if the 100 size is a good one.

Andres, would you mind making a couple of changes:

(a) Make the size of the LRU cache configurable via an environment variable MONO_ASPNET_WEBCONFIG_CACHESIZE

(b) Alter LRUCache.cs to track the effectiveness of the LRUcache, and report (only once) if the effectiveness drops that the user can fix this by increasing the size of the above variable.

To track the effectiveness, I suggest that we add a variable that tracks evictions:

Add ()
{
  if (...){
     Line 110: before the return when we return a value from the cache

    evictLevel = 0;
    return;
  }
  if (xxx > yyy){
     evictions++;
     Evict ();

    if (need_warning && evictions >= CACHE_SIZE){
       need_warning = false;
       printf ("hey, try increasing the cache size");
    }
  }
}
Comment 11 Andres G. Aragoneses 2013-04-23 12:14:31 UTC
(In reply to comment #7)
> track the effectiveness of the LRUcache, and report
> (only once) if the effectiveness drops that the user can fix this by increasing
> the size of the above variable.

I see how this would come in handy for normal ASP.NET apps with non-variable URIs, but for the cases which were hitting this bug already (variable virtual paths), we would see this warning printed AFAIU, which could maybe be a bit confusing and thus make the user increase the default size (which will have no effect other than seeing the warning a bit later). Or am I missing something?
Comment 12 Andres G. Aragoneses 2013-04-23 12:15:38 UTC
other than seeing the warning a bit later -> *and* increase memory usage obviously
Comment 13 Miguel de Icaza [MSFT] 2013-04-23 12:28:21 UTC
The warning is useful, because it helps people keep their app performant, even with variable URIs.

We need to empower developers, not force them to go spelunking again and find out why their app is crawling.
Comment 14 Andres G. Aragoneses 2013-04-23 12:43:54 UTC
Ok, I'll add the change, but 2 more questions:

1) Should I use Console.WriteLine or Console.Error.WriteLine?
2) What should be the exact message? I'm thinking "WebConfigurationManager reached maximum length of the cache, WebConfigurationManager's LRUcache eviction performed\nCurrent Cache Size: {0} (overridable via MONO_ASPNET_WEBCONFIG_CACHESIZE)"

Thanks
Comment 15 Andres G. Aragoneses 2013-04-23 12:44:42 UTC
Oops, I meant to put a shorter message:

"WebConfigurationManager's LRUcache eviction performed
Current Cache Size: {0} (overridable via MONO_ASPNET_WEBCONFIG_CACHESIZE)"
Comment 16 Miguel de Icaza [MSFT] 2013-04-23 13:44:36 UTC
(1) Let us use Console.WriteLine, pending Marek's opinion on this.

(2) Love the message.
Comment 17 Marek Habersack 2013-04-24 04:04:51 UTC
Console.Error.WriteLine is better since it will end up in the web server's log - mod_mono swallows stdout output if not running in debug mode. Also, see my comments in the pull request.
Comment 18 Andres G. Aragoneses 2013-04-24 19:05:53 UTC
Ok thanks for the answers; implemented, and updated pull request.

NOTE: I decided to do the env var parsing in LruCache class instead of in WebConfigurationManager. You may think that LruCache should be generic enough for other code to use it, but:
a) If we were to be so "strict", then I would need to also implement the "eviction warning" logic as an event (which WebConfigurationManager would subscribe to), so WebConfigurationManager prints the warning and not the "generic LruCache".
b) Right now WebConfigurationManager would be the only user.

Some more clarifications and questions down here:

(In reply to comment #10)
>    evictLevel = 0;

Hey Miguel sorry to come back to this, but you didn't use "evictLevel" variable after this line in your snippet, what did you mean by this?
Anyway I've implemented a first version of this in the 3rd commit of my pull request. Let me know if I should add the evictLevel?

(In reply to comment #17)
> Console.Error.WriteLine is better since it will end up in the web server's log

Mmmmmh..., but this is not really an error, strictly speaking. It's a warning. Sadly there is only STDOUT and STDERR but no STDWARN, we could get philosophical on this, but at this point, now I think it would be better to use Console.WriteLine to not clover error logs in production (because if you're not really testing your code --debug mode-- before putting it in production, YouReDoingItWrongTM).

(In reply to pullrequest)
> Andres, when you add the envvar or the app settings key support - please update the
> mod_mono and mono manpages

Done in my last commit of the pull request..

(In reply to pullrequest)
> Andres, also a itsy-bitsy nit-pick... :) You don't need the 'internal' keyword in class definition :)

Fixed in my last update to the pull request.

(In reply to pullrequest)
> - I think adding support for AppSetting (MonoWebconfigCacheSize for instance) would be a better
> approach. Environment variables are fine, but most Apache installations clean the environment
> before they load modules and I've heard from people running into this issue (they can set the
> variable from the vhost config for the Mono app, but it's less intuitive than just adding an
> AppSetting to web.config)

Well, this is growing more like into a feature request. To be honest I'm not using Apache so it's not really a scenario I care about right now (we use nginx, which AFAIU scales much better). How about if I implement the env-var based approach, and the next contributor can improve it from that point to use an appSetting?

> - mod_mono swallows stdout output if not running in debug mode. Also, see my
> comments in the pull request.

This wouldn't affect us either because we don't use mod_mono.
I'm happy to create a new bug for translating the env var into an appsetting, and I may revisit it in the future.
Comment 19 Marek Habersack 2013-04-25 02:32:50 UTC
(In reply to comment #18)
> Ok thanks for the answers; implemented, and updated pull request.
> 
> NOTE: I decided to do the env var parsing in LruCache class instead of in
> WebConfigurationManager. You may think that LruCache should be generic enough
> for other code to use it, but:
> a) If we were to be so "strict", then I would need to also implement the
> "eviction warning" logic as an event (which WebConfigurationManager would
> subscribe to), so WebConfigurationManager prints the warning and not the
> "generic LruCache".
> b) Right now WebConfigurationManager would be the only user.
> 
> Some more clarifications and questions down here:
> 
> (In reply to comment #10)
> >    evictLevel = 0;
> 
> Hey Miguel sorry to come back to this, but you didn't use "evictLevel" variable
> after this line in your snippet, what did you mean by this?
> Anyway I've implemented a first version of this in the 3rd commit of my pull
> request. Let me know if I should add the evictLevel?
> 
> (In reply to comment #17)
> > Console.Error.WriteLine is better since it will end up in the web server's log
> 
> Mmmmmh..., but this is not really an error, strictly speaking. It's a warning.
> Sadly there is only STDOUT and STDERR but no STDWARN, we could get
> philosophical on this, but at this point, now I think it would be better to use
> Console.WriteLine to not clover error logs in production (because if you're not
> really testing your code --debug mode-- before putting it in production,
> YouReDoingItWrongTM).
You'd think so, but in my experience most of those problems manifest themselves in production and for the user it will be just an entry in the web server log - no indication that it comes from stderr. The warning's important enough to warrant making sure it gets logged in production.

> 
> (In reply to pullrequest)
> > Andres, when you add the envvar or the app settings key support - please update the
> > mod_mono and mono manpages
> 
> Done in my last commit of the pull request..
Cool, thanks

> 
> (In reply to pullrequest)
> > Andres, also a itsy-bitsy nit-pick... :) You don't need the 'internal' keyword in class definition :)
> 
> Fixed in my last update to the pull request.
Thanks :)

> 
> (In reply to pullrequest)
> > - I think adding support for AppSetting (MonoWebconfigCacheSize for instance) would be a better
> > approach. Environment variables are fine, but most Apache installations clean the environment
> > before they load modules and I've heard from people running into this issue (they can set the
> > variable from the vhost config for the Mono app, but it's less intuitive than just adding an
> > AppSetting to web.config)
> 
> Well, this is growing more like into a feature request. To be honest I'm not
> using Apache so it's not really a scenario I care about right now (we use
> nginx, which AFAIU scales much better). How about if I implement the env-var
> based approach, and the next contributor can improve it from that point to use
> an appSetting?
Fair enough.

> 
> > - mod_mono swallows stdout output if not running in debug mode. Also, see my
> > comments in the pull request.
> 
> This wouldn't affect us either because we don't use mod_mono.
> I'm happy to create a new bug for translating the env var into an appsetting,
> and I may revisit it in the future.
No need, I'll implement the appsetting once the PR is accepted
Comment 20 Miguel de Icaza [MSFT] 2013-04-25 09:22:02 UTC
The reading of the environment variable should be on the WebConfigurationManager, not on the LRU cache.   You might want differnet caches to have different sizes, so it makes no sense to have a central place to configure it.
Comment 21 Andres G. Aragoneses 2013-04-25 09:27:50 UTC
(In reply to comment #18)
> The reading of the environment variable should be on the
> WebConfigurationManager, not on the LRU cache.   You might want differnet
> caches to have different sizes, so it makes no sense to have a central place to
> configure it.

Alright, did you read my "NOTE" on comment#21? If yes, then I'm assuming you're fine with me implement events then, so WebConfigurationManager is the one calling Console.WriteLine () and not LRUcache (by using the same logic).

But now that Marek said he's going to implement it via AppSetting, I guess it's pointless that I do it?
Comment 22 Miguel de Icaza [MSFT] 2013-04-25 09:55:51 UTC
We dont need an event, we just need a user supplied callback.

Set a public property "string DebugTrashingMessage"
Comment 23 Andres G. Aragoneses 2013-04-25 10:04:20 UTC
Fair enough. Then waiting for Marek's green light before I finish that.
Comment 24 Marek Habersack 2013-04-26 02:36:25 UTC
Go ahead! And, please, switch to Console.Error.WriteLine () :-)
Comment 25 Andres G. Aragoneses 2013-04-26 12:09:18 UTC
(In reply to comment #22)
> Set a public property

Alright, done.


(In reply to comment #24)
> Go ahead! And, please, switch to Console.Error.WriteLine () :-)

Done too, pull-request should have everything that was requested.
Comment 26 Miguel de Icaza [MSFT] 2013-04-26 12:47:31 UTC
Thank you!

Pull request merged
Comment 27 Bojan Rajkovic [MSFT] 2013-05-01 16:18:46 UTC
Hi guys,

I hate to be the bearer of bad news, but this patch is bad. It causes the following message to randomly crop up using the tip of mono/master:

System.InvalidOperationException: Operation is not valid due to the current state of the object
  at System.Collections.Generic.LinkedList`1[System.Object].VerifyReferencedNode (System.Collections.Generic.LinkedListNode`1 node) [0x0001d] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-3.0.11/mcs/class/System/System.Collections.Generic/LinkedList.cs:75 
  at System.Collections.Generic.LinkedList`1[System.Object].Remove (System.Collections.Generic.LinkedListNode`1 node) [0x00000] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-3.0.11/mcs/class/System/System.Collections.Generic/LinkedList.cs:299 
  at System.Web.Configuration.LruCache`2[System.Int32,System.Object].TryGetValue (Int32 key, System.Object& value) [0x00013] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-3.0.11/mcs/class/System.Web/System.Web.Configuration_2.0/LruCache.cs:102 
  at System.Web.Configuration.WebConfigurationManager.GetSection (System.String sectionName, System.String path, System.Web.HttpContext context) [0x0009a] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-3.0.11/mcs/class/System.Web/System.Web.Configuration_2.0/WebConfigurationManager.cs:482 
  at System.Web.Configuration.WebConfigurationManager.GetSection (System.String sectionName) [0x00006] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-3.0.11/mcs/class/System.Web/System.Web.Configuration_2.0/WebConfigurationManager.cs:432 
  at Mono.WebServer.BaseApplicationHost.LocateHandler (System.String verb, System.String uri) [0x00001] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-xsp-d3e2f80/src/Mono.WebServer/BaseApplicationHost.cs:188 
  at Mono.WebServer.BaseApplicationHost.IsHttpHandler (System.String verb, System.String uri) [0x0006f] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-xsp-d3e2f80/src/Mono.WebServer/BaseApplicationHost.cs:168 
  at Mono.WebServer.Paths.VirtualPathExists (IApplicationHost appHost, System.String verb, System.String uri) [0x00001] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-xsp-d3e2f80/src/Mono.WebServer/Paths.cs:90 
  at Mono.WebServer.Paths.GetPathsFromUri (IApplicationHost appHost, System.String verb, System.String uri, System.String& realUri, System.String& pathInfo) [0x000b1] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-xsp-d3e2f80/src/Mono.WebServer/Paths.cs:79 
  at Mono.WebServer.XSPWorkerRequest..ctor (Int32 requestId, Mono.WebServer.XSPRequestBroker requestBroker, IApplicationHost appHost, System.Net.EndPoint localEP, System.Net.EndPoint remoteEP, System.String verb, System.String path, System.String queryString, System.String protocol, System.Byte[] inputBuffer, IntPtr socket, Boolean secure) [0x0005b] in /private/tmp/source/bockbuild-mono/profiles/mono-mac-xamarin/build-root/mono-xsp-d3e2f80/src/Mono.WebServer.XSP/XSPWorkerRequest.cs:184 

After the first appearance of the error, pages error out subsequently on every load.
Comment 28 Miguel de Icaza [MSFT] 2013-05-01 16:42:19 UTC
Let us revert for now.
Comment 29 Andres G. Aragoneses 2013-05-01 16:46:40 UTC
Oops, sorry, my first guess is a race condition, so I guess I didn't test in enough anger...

But how a race condition if there was already locking around the sectionCache object? Well, I just realised that there were 2 types of locking: for readers and for writers. But since using a LruCache, a read access also modifies the collection, so I guess this is the culprit I/we overlooked.

I'll look at cooking a fix. What would be preferable? Convert this to only 1 writable lock? I was thinking we could have some "nominations-subqueue" that would be used for readings, and when there is a write, flush the queue, but that would probably involve a lock inside LruCache to handle the access to this subqueue...
Comment 30 Bojan Rajkovic [MSFT] 2013-05-01 16:54:47 UTC
Andres, I'm going to revert the patch for now and reopen the pull request. I'll retest with a fixed patch that hopefully doesn't cause this. :-)
Comment 31 Andres G. Aragoneses 2013-05-01 16:59:04 UTC
Fine by me. BTW, I guess the revert should be ideally be done with 1 commit, not N (the PR had more than 1 commit FYI).
Comment 32 Bojan Rajkovic [MSFT] 2013-05-01 17:06:24 UTC
Yep, I'm doing it the right way. :) git revert has gotten smarter about merges sometime recently.
Comment 33 Marek Habersack 2013-05-06 09:33:07 UTC
Andres, use an upgradeable rw lock.
Comment 34 Andres G. Aragoneses 2013-05-14 12:12:44 UTC
(In reply to comment #33)
> Andres, use an upgradeable rw lock.

Thing is, this kind of lock is already being used (in particular, ReaderWriterLockSlim, which I guess is upgradeable because it contains the method "TryEnterUpgradeableReadLock" in its API, which is being used right now).

However, to reiterate what I said in comment#29, I think the problem that Bojan saw is precisely because of using this lock: all read accesses now modify the collection (to modify the order of items in the linked list), so then I need to modify all lock accesses to be write accesses.

I'm going to go ahead with this plan now unless I get any objections.
Comment 35 Marek Habersack 2013-05-14 16:41:59 UTC
I guess we can go on with write locks then... It will probably hurt performance a little, but to be absolutely sure we'd need to run some real-life tests. Go ahead with write lock (but do use ReaderWriterLockSlim - it's much faster than rwlock)
Comment 36 Andres G. Aragoneses 2013-05-24 13:56:49 UTC
(In reply to comment #35)
> I guess we can go on with write locks then... It will probably hurt performance
> a little,

Well, this bug is about a memory leak in a cache, a cache is a performance optimization, so then what really happens here is that we're fixing the performance optimization to be more correct (and because of that, the optimization is a little less great) :)

> but to be absolutely sure we'd need to run some real-life tests.

Bojan has tested my new pull request and it doesn't give the problems he mentioned in comment#27.

> Go ahead with write lock (but do use ReaderWriterLockSlim - it's much faster
> than rwlock)

Done, it's here:
https://github.com/mono/mono/pull/647

Thanks
Comment 37 Marek Habersack 2013-05-27 03:04:19 UTC
(In reply to comment #36)
> (In reply to comment #35)
> > I guess we can go on with write locks then... It will probably hurt performance
> > a little,
> 
> Well, this bug is about a memory leak in a cache, a cache is a performance
> optimization, so then what really happens here is that we're fixing the
> performance optimization to be more correct (and because of that, the
> optimization is a little less great) :)
Sure, but correctness comes over performance and sometimes you don't have a choice :)) - especially if you're coding a library to be used by 3rd parties.

> 
> > but to be absolutely sure we'd need to run some real-life tests.
> 
> Bojan has tested my new pull request and it doesn't give the problems he
> mentioned in comment#27.
> 
> > Go ahead with write lock (but do use ReaderWriterLockSlim - it's much faster
> > than rwlock)
> 
> Done, it's here:
> https://github.com/mono/mono/pull/647
There's one little thing I don't like in this pull request - commented inline. Besides that, it's great and almost ready to be merged :)

marek
> 
> Thanks
Comment 38 Marek Habersack 2013-05-27 16:36:32 UTC
And merged! Thanks Andres :)