Mailing-List: contact solr-dev-help@lucene.apache.org; run by ezmlm
Precedence: bulk
Reply-To: solr-dev@lucene.apache.org
Received-SPF: pass (nike.apache.org: domain of ryantxu@gmail.com designates
 74.125.92.24 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:from:to:in-reply-to:content-type
         :content-transfer-encoding:mime-version:subject:date:references
         :x-mailer;
        b=jIhqwWG3H+OsNDo5bL5dz1ws/rtwCXpYwVJ7FQWIHx5Hlkp4F0eMt6R32VK3ea1SWk
         3VqrHHXHqoY40yReT42HrFTVmz7KZ+EpSrGSjzatJPy3qqSrJoFf+MiGsf0xu9KLg6ys
         xm1AglpBTQach6Dd5VYUj5YuY2+dY3kqEgtVw=
Message-Id: <BB87F6EA-7C6E-4274-A6CC-3D8659414606@gmail.com>
From: Ryan McKinley <ryantxu@gmail.com>
To: solr-dev@lucene.apache.org
In-Reply-To: <597c69661001070723q58e9859l6efe3a83f55d1015@mail.gmail.com>
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0 (Apple Message framework v936)
Subject: Re: Race condition in
 org/apache/solr/client/solrj/impl/StreamingUpdateSolrServer
Date: Thu, 7 Jan 2010 16:36:58 -0500
References: <597c69661001070723q58e9859l6efe3a83f55d1015@mail.gmail.com>

can  you submit a patch to JIRA?


On Jan 7, 2010, at 10:23 AM, Attila Babo wrote:

> While inserting a large pile of documents using
> StreamingUpdateSolrServer I've found a race condition as all Runner
> instances stopped while the blocking queue was full. The attached
> patch solves the problem, to minify it all indentation has been
> removed.
>
> Index: src/solrj/org/apache/solr/client/solrj/impl/ 
> StreamingUpdateSolrServer.java
> ===================================================================
> --- src/solrj/org/apache/solr/client/solrj/impl/ 
> StreamingUpdateSolrServer.java	(revision
> 888167)
> +++ src/solrj/org/apache/solr/client/solrj/impl/ 
> StreamingUpdateSolrServer.java	(working
> copy)
> @@ -82,6 +82,7 @@
>       log.info( "starting runner: {}" , this );
>       PostMethod method = null;
>       try {
> +        do {
>         RequestEntity request = new RequestEntity() {
>           // we don't know the length
>           public long getContentLength() { return -1; }
> @@ -142,6 +143,7 @@
>           msg.append( "request: "+method.getURI() );
>           handleError( new Exception( msg.toString() ) );
>         }
> +        }  while( ! queue.isEmpty());
>       }
>       catch (Throwable e) {
>         handleError( e );
> @@ -149,6 +151,7 @@
>       finally {
>         try {
>           // make sure to release the connection
> +          if(method != null)
>           method.releaseConnection();
>         }
>         catch( Exception ex ){}
> @@ -195,11 +198,11 @@
>
>       queue.put( req );
>
> +        synchronized( runners ) {
>       if( runners.isEmpty()
>         || (queue.remainingCapacity() < queue.size()
>          && runners.size() < threadCount) )
>       {
> -        synchronized( runners ) {
>           Runner r = new Runner();
>           scheduler.execute( r );
>           runners.add( r );
>
> ===================
>
> This patch has been tested with millions of document inserted to Solr,
> before that I was unable to inject all of our documents as the
> following scenario happened. We have a BlockingQueue called runners to
> handle requests, at one point the queue was emptied by the Runner
> threads, they all stopped processing new items but sent the collected
> items to Solr. Solr was busy so that toke a long time, during that the
> client filled the queue again. As all worker threads were instantiated
> there were no way to create new Runners to handle the queue so it was
> growing to upper limit. When the next item was about to put into the
> queue it was blocked and the race condition just happened.
>
> Patch 1, 2:
> Inside the Runner.run method I've added a do while loop to prevent the
> Runner to quit while there are new requests, this handles the problem
> of new requests added while Runner is sending the previous batch.
>
> Patch 3
> Validity check of method variable is not strictly necessary, just a
> code clean up.
>
> Patch 4
> The last part of the patch is to move synchronized outside of
> conditional to avoid a situation where runners change while evaluating
> it.
>
> Your comments and critique are welcome!
>
> Attila