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

[Gonzalo Paniagua Javier]

El jue, 20-11-2003 a las 01:45, eric lindvall escribió:
> comments inlined:
> 
> On Wed, 19 Nov 2003, Gonzalo Paniagua Javier wrote:
> 
> > El mié, 19-11-2003 a las 18:35, eric lindvall escribió:
> > > sorry, forgot to cvs update first.
> > 
> > +		lock (typeof (DefaultConfig)) {
> >  			if (instance == null) {
> > -				instance = new DefaultConfig ();
> > -				instance.Init ();
> > +                               Interlocked.Exchange (
> > +                              ref instance, new DefaultConfig());}
> > -					
> > 
> > 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
        }
        
> 
> 
> > 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...

> 
> 
> > Also if I try to run an application that uses ConfigurationSettings and
> > there's no application configuration file (ie, only machine.config is
> > read), i get an exception:
> > 
> > Unhandled Exception: System.IO.FileNotFoundException: Could not find
> > file "/home/gpanjav/go-mono/appsettings.exe.config"
> > 
> > This should not happen.
> 
> correct. i'm not sure why i didn't find that when i was running my tests
> (i didn't have an app.config file either).

Okay. You should test xsp a bit when modifying this stuff, with and
without web.config file.

-Gonzalo