synchronization issue in SolrCore.getSolrCore() ?

[Chris Hostetter <ho...@fucit.org>]

I may be way off here, but with the changes made to support multiple 
cores, isn't there a possible race condition in the default single core 
situation between the assignment to "private static SolrCore instance" in 
SolrCore.getSolrCore() and the SolrCore constructor?

Consider the case where legacy code calls SolrCore.getSolrCore() while 
another thread is in the middle of executing the SolrCore constructor 
(either directly, or because it called to getSolrCore()) ... won't it have 
access to a "partially constructed" core before all of the initialization 
is complete?

It seems like either...

1) getSolrCore() should also synchronize on SolrCore.class
...OR...
2) SolrCore(...) should not include the "instance = this" line, we put 
the synchronization block back into getSolrCore() and make it the only 
place instance is assigned ... change it's documentation to make it clear 
that it generates a new "anonymouns" SolrCore the first time it's called 
and allways returns that same SolrCore


does that make sense?


(forgive me if i'm way off base and this is something that was allready 
thoroughly discussed in the multi-core bugs/threads ... i wasn't able to 
followthem that closely and only stumbled upon this while looking at 
something else)


-Hoss

Re: synchronization issue in SolrCore.getSolrCore() ?

[Ryan McKinley <ry...@gmail.com>]

> 
> i'm always a fan of quick fixes ... even if they are only temporary ... 
> but like i said, i'm confused as to why you wanted to avoid #1 or why #2 
> would breakthings.

sorry, my object to #1 was a synchronized map lookup.  a simple 
synchronized call should not hurt.

#2 breaks the Jetty test -- I'm looking for a way to rewrite it that 
does not, but nothing is obvious yet.  The basic issue is that the 
testHarness creates a new SolrCore for each test.  Jetty runner is 
initialized with:
  root.addFilter( SolrDispatchFilter.class, "*", Handler.REQUEST );

then jetty instantiates the DispatchFilter which calls getSolrCore() and 
needs to get the *last* one initialized.

- - - - -

I'll commit option #1, and keep an eye on this for SOLR-350

ryan

Re: synchronization issue in SolrCore.getSolrCore() ?

[Chris Hostetter <ho...@fucit.org>]

: > 1) getSolrCore() should also synchronize on SolrCore.class
: 
: This is the big thing I wanted to avoid.

why?  that's the way the old impl worked right?

: I tried this originally, but it breaks all the tests -- the tests require the
: last constructed core is returned form getSolrCore()

that seems a little scary ... which tests?

tests of code that assume a multi core enviornment should never be using 
getSolrCore() ... legacy tests shouldn't care which core they get, as long 
as it's always the same.

: The only quick fix is #1 -- should we do that now or wait till SOLR-350?

: I'm inclined to wait and keep an eye on SOLR-350.

i'm always a fan of quick fixes ... even if they are only temporary ... 
but like i said, i'm confused as to why you wanted to avoid #1 or why #2 
would breakthings.

In general: if you've got your eye on this as part of SOLR-350 then i'm 
not that worried.  carry on good sir, carry on.


-Hoss

Re: synchronization issue in SolrCore.getSolrCore() ?

[Ryan McKinley <ry...@gmail.com>]

> 
> Consider the case where legacy code calls SolrCore.getSolrCore() while 
> another thread is in the middle of executing the SolrCore constructor 
> (either directly, or because it called to getSolrCore()) ... won't it 
> have access to a "partially constructed" core before all of the 
> initialization is complete?
> 

Yes, that could happen (Not 'off the shelf').  In SOLR-350, I *think* I 
adressed this.  The core registry is extracted from SolrCore and is 
careful to only set the default core after it is constructed.


> It seems like either...
> 
> 1) getSolrCore() should also synchronize on SolrCore.class

This is the big thing I wanted to avoid.


> ...OR...
> 2) SolrCore(...) should not include the "instance = this" line, we put 
> the synchronization block back into getSolrCore() and make it the only 
> place instance is assigned ... change it's documentation to make it 
> clear that it generates a new "anonymouns" SolrCore the first time it's 
> called and allways returns that same SolrCore
> 

I tried this originally, but it breaks all the tests -- the tests 
require the last constructed core is returned form getSolrCore()


> 
> does that make sense?
> 

The only quick fix is #1 -- should we do that now or wait till SOLR-350?

I'm inclined to wait and keep an eye on SOLR-350.

thoughts?
ryan