Re: Synchronization in SolrClientCache looks wrong
Right, thanks. It was a rushed day yesterday and misread "object" in the javadocs. Also misdirected by the fact that I had a slightly different version of some code that produces an NPE and was thinking that the problem was closing a null SolrClient instead of the variable two lines above it. Never mind. On Fri, Aug 11, 2017 at 8:45 AM, Tomas Fernandez Lobbe wrote: > The synchronized at the method level blocks any other call to any > synchronized method on the same object, so this is correct. > >> On Aug 10, 2017, at 4:34 PM, Erick Erickson wrote: >> >> And if it's impossible for more than one thread to be using the class, >> why bother to synchronize? >> >> Or am I misreading things totally? >> >> On Thu, Aug 10, 2017 at 4:21 PM, Erick Erickson >> wrote: >>> What prevents one thread from calling a get operation while another is >>> closing out from underneath? Or is it impossible that more than one thread >>> have access to this object? >>> >>> >>> On Aug 10, 2017 2:32 PM, "Joel Bernstein" wrote: >>> >>> Just took a look at the class and it seems OK. All methods are synchronized, >>> so we shouldn't have any race conditions. What are your concerns? >>> >>> Joel Bernstein >>> http://joelsolr.blogspot.com/ >>> >>> On Thu, Aug 10, 2017 at 4:32 PM, Tomas Fernandez Lobbe >>> wrote: You mean, remove the synchronization from the method and synchronize the whole thing on solrClients? How is that different? > On Aug 10, 2017, at 12:48 PM, Erick Erickson > wrote: > > All the methods are synchronized, but they all operate on the > > private final Map solrClients; > > member variable, including code like this: > > if (solrClients.containsKey(host)) { > client = (HttpSolrClient) solrClients.get(host); > } > > But the close method goes through the list closing all the entries in > solrClients. Seems like these ought to be synchronized blocks on > solrClients > > I have another place that needs updating too that I was investigating > when I saw this so I'll do them both at once if so (SOLR-11224) > > Erick > > - > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org >>> >>> >> >> - >> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org >> For additional commands, e-mail: dev-h...@lucene.apache.org >> > > > - > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Synchronization in SolrClientCache looks wrong
The synchronized at the method level blocks any other call to any synchronized method on the same object, so this is correct. > On Aug 10, 2017, at 4:34 PM, Erick Erickson wrote: > > And if it's impossible for more than one thread to be using the class, > why bother to synchronize? > > Or am I misreading things totally? > > On Thu, Aug 10, 2017 at 4:21 PM, Erick Erickson > wrote: >> What prevents one thread from calling a get operation while another is >> closing out from underneath? Or is it impossible that more than one thread >> have access to this object? >> >> >> On Aug 10, 2017 2:32 PM, "Joel Bernstein" wrote: >> >> Just took a look at the class and it seems OK. All methods are synchronized, >> so we shouldn't have any race conditions. What are your concerns? >> >> Joel Bernstein >> http://joelsolr.blogspot.com/ >> >> On Thu, Aug 10, 2017 at 4:32 PM, Tomas Fernandez Lobbe >> wrote: >>> >>> You mean, remove the synchronization from the method and synchronize the >>> whole thing on solrClients? How is that different? >>> >>> On Aug 10, 2017, at 12:48 PM, Erick Erickson wrote: All the methods are synchronized, but they all operate on the private final Map solrClients; member variable, including code like this: if (solrClients.containsKey(host)) { client = (HttpSolrClient) solrClients.get(host); } But the close method goes through the list closing all the entries in solrClients. Seems like these ought to be synchronized blocks on solrClients I have another place that needs updating too that I was investigating when I saw this so I'll do them both at once if so (SOLR-11224) Erick - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org >>> >>> >>> - >>> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org >>> For additional commands, e-mail: dev-h...@lucene.apache.org >>> >> >> > > - > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Synchronization in SolrClientCache looks wrong
And if it's impossible for more than one thread to be using the class, why bother to synchronize? Or am I misreading things totally? On Thu, Aug 10, 2017 at 4:21 PM, Erick Erickson wrote: > What prevents one thread from calling a get operation while another is > closing out from underneath? Or is it impossible that more than one thread > have access to this object? > > > On Aug 10, 2017 2:32 PM, "Joel Bernstein" wrote: > > Just took a look at the class and it seems OK. All methods are synchronized, > so we shouldn't have any race conditions. What are your concerns? > > Joel Bernstein > http://joelsolr.blogspot.com/ > > On Thu, Aug 10, 2017 at 4:32 PM, Tomas Fernandez Lobbe > wrote: >> >> You mean, remove the synchronization from the method and synchronize the >> whole thing on solrClients? How is that different? >> >> >> > On Aug 10, 2017, at 12:48 PM, Erick Erickson >> > wrote: >> > >> > All the methods are synchronized, but they all operate on the >> > >> > private final Map solrClients; >> > >> > member variable, including code like this: >> > >> > if (solrClients.containsKey(host)) { >> > client = (HttpSolrClient) solrClients.get(host); >> > } >> > >> > But the close method goes through the list closing all the entries in >> > solrClients. Seems like these ought to be synchronized blocks on >> > solrClients >> > >> > I have another place that needs updating too that I was investigating >> > when I saw this so I'll do them both at once if so (SOLR-11224) >> > >> > Erick >> > >> > - >> > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org >> > For additional commands, e-mail: dev-h...@lucene.apache.org >> > >> >> >> - >> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org >> For additional commands, e-mail: dev-h...@lucene.apache.org >> > > - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Synchronization in SolrClientCache looks wrong
What prevents one thread from calling a get operation while another is closing out from underneath? Or is it impossible that more than one thread have access to this object? On Aug 10, 2017 2:32 PM, "Joel Bernstein" wrote: Just took a look at the class and it seems OK. All methods are synchronized, so we shouldn't have any race conditions. What are your concerns? Joel Bernstein http://joelsolr.blogspot.com/ On Thu, Aug 10, 2017 at 4:32 PM, Tomas Fernandez Lobbe wrote: > You mean, remove the synchronization from the method and synchronize the > whole thing on solrClients? How is that different? > > > > On Aug 10, 2017, at 12:48 PM, Erick Erickson > wrote: > > > > All the methods are synchronized, but they all operate on the > > > > private final Map solrClients; > > > > member variable, including code like this: > > > > if (solrClients.containsKey(host)) { > > client = (HttpSolrClient) solrClients.get(host); > > } > > > > But the close method goes through the list closing all the entries in > > solrClients. Seems like these ought to be synchronized blocks on > > solrClients > > > > I have another place that needs updating too that I was investigating > > when I saw this so I'll do them both at once if so (SOLR-11224) > > > > Erick > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > > For additional commands, e-mail: dev-h...@lucene.apache.org > > > > > - > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org > >
Re: Synchronization in SolrClientCache looks wrong
Just took a look at the class and it seems OK. All methods are synchronized, so we shouldn't have any race conditions. What are your concerns? Joel Bernstein http://joelsolr.blogspot.com/ On Thu, Aug 10, 2017 at 4:32 PM, Tomas Fernandez Lobbe wrote: > You mean, remove the synchronization from the method and synchronize the > whole thing on solrClients? How is that different? > > > > On Aug 10, 2017, at 12:48 PM, Erick Erickson > wrote: > > > > All the methods are synchronized, but they all operate on the > > > > private final Map solrClients; > > > > member variable, including code like this: > > > > if (solrClients.containsKey(host)) { > > client = (HttpSolrClient) solrClients.get(host); > > } > > > > But the close method goes through the list closing all the entries in > > solrClients. Seems like these ought to be synchronized blocks on > > solrClients > > > > I have another place that needs updating too that I was investigating > > when I saw this so I'll do them both at once if so (SOLR-11224) > > > > Erick > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > > For additional commands, e-mail: dev-h...@lucene.apache.org > > > > > - > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org > >
Re: Synchronization in SolrClientCache looks wrong
You mean, remove the synchronization from the method and synchronize the whole thing on solrClients? How is that different? > On Aug 10, 2017, at 12:48 PM, Erick Erickson wrote: > > All the methods are synchronized, but they all operate on the > > private final Map solrClients; > > member variable, including code like this: > > if (solrClients.containsKey(host)) { > client = (HttpSolrClient) solrClients.get(host); > } > > But the close method goes through the list closing all the entries in > solrClients. Seems like these ought to be synchronized blocks on > solrClients > > I have another place that needs updating too that I was investigating > when I saw this so I'll do them both at once if so (SOLR-11224) > > Erick > > - > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org