[Mono-devel-list] proposed patch for System.Configuration.ConfigurationSetting

[Gonzalo Paniagua Javier]

El jue, 20-11-2003 a las 06:25, eric lindvall escribió:
> inlined again.
> 
> On Thu, 20 Nov 2003, Gonzalo Paniagua Javier wrote:
> 
> > > > There's no need to use Interlocked here, as we're already inside a lock.
> > > 
> > > isn't it going to be possible that the first "instance == null" check
> > > could find a half-assigned object ref? i debated that one for a bit, but
> > > it seems it is "more correct" to do the exchange.
> > 
> > You're already inside the lock. That could happen if you do:
> >         if (instace != null)
> >         	return instace;
> >         lock (typeof (DefaultConfig) {
> >         	blah
> >         }
> 
> 
> the full method is:
> 
> public static DefaultConfig GetInstance ()
> {
>         if (instance == null) {
>                 lock (typeof (DefaultConfig)) {
>                         if (instance == null) {
>                                 Interlocked.Exchange (
>                                                 ref instance, new DefaultConfig());
>                         }
>                 }
>         }
> 
>         return ((DefaultConfig) instance);
> }
> 
> it does the first check so we don't need to lock every time we try to get
> any configuration setting.

Then we should remove the first "if"... I know we do that in many
places, but see
http://research.microsoft.com/~birrell/papers/ThreadsCSharp.pdf for
reasons not to do that.

> 
> 
> > > > IMO, the changes to GetDocumentForSection are not needed. I vote for
> > > > leaving the current code as it is.
> > > 
> > > the filename check is now completely redundant, and there are cases (if
> > > you get exceptions from within the method) where we will leak fd's if you
> > > don't use a using() statement (or try/finally).
> > 
> > If we leak fd's, then the GC is not doing its work. The GC will call the
> > FileStream finalizer in case of an exception in between. We don't set
> > the fd free inmediately, but well...
> 
> the only other objection i have to the current implementation of
> GetDocumentForSection() is the returning null instead of allowing the
> exception to be thrown if you can't load the file.

That change you did is probably the cause of that exception i reported
in the previous message.

> > 
> > Okay. You should test xsp a bit when modifying this stuff, with and
> > without web.config file.
> 
> are there any specific tests you would suggest? i've tried renaming
> web.config as well as xsp.exe.config and 

No, just running it and try 1 aspx + 1 asmx should be ok. Then try the
same without web.config file.

-Gonzalo