Synchronization in SolrClientCache looks wrong

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Synchronization in SolrClientCache looks wrong

Erick Erickson
All the methods are synchronized, but they all operate on the

private final Map<String, SolrClient> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Synchronization in SolrClientCache looks wrong

Tomas Fernandez Lobbe-2
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 <[hidden email]> wrote:
>
> All the methods are synchronized, but they all operate on the
>
> private final Map<String, SolrClient> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Synchronization in SolrClientCache looks wrong

Joel Bernstein
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?


On Thu, Aug 10, 2017 at 4:32 PM, Tomas Fernandez Lobbe <[hidden email]> 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 <[hidden email]> wrote:
>
> All the methods are synchronized, but they all operate on the
>
> private final Map<String, SolrClient> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Synchronization in SolrClientCache looks wrong

Erick Erickson
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" <[hidden email]> 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?


On Thu, Aug 10, 2017 at 4:32 PM, Tomas Fernandez Lobbe <[hidden email]> 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 <[hidden email]> wrote:
>
> All the methods are synchronized, but they all operate on the
>
> private final Map<String, SolrClient> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Synchronization in SolrClientCache looks wrong

Erick Erickson
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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]>
>> > wrote:
>> >
>> > All the methods are synchronized, but they all operate on the
>> >
>> > private final Map<String, SolrClient> 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: [hidden email]
>> > For additional commands, e-mail: [hidden email]
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Synchronization in SolrClientCache looks wrong

Tomas Fernandez Lobbe-2
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 <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]>
>> 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 <[hidden email]>
>>>> wrote:
>>>>
>>>> All the methods are synchronized, but they all operate on the
>>>>
>>>> private final Map<String, SolrClient> 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: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Synchronization in SolrClientCache looks wrong

Erick Erickson
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
<[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]>
>>> 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 <[hidden email]>
>>>>> wrote:
>>>>>
>>>>> All the methods are synchronized, but they all operate on the
>>>>>
>>>>> private final Map<String, SolrClient> 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: [hidden email]
>>>>> For additional commands, e-mail: [hidden email]
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...