hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rezaei, Mohammad A." <mohammad.rez...@gs.com>
Subject RE: [Bug 25370] - exception during writeRequest leaves the conn ection un-released
Date Thu, 11 Dec 2003 15:57:14 GMT
I'm forwarding this to the list. The conversation that originally started on
Bug 25370 took a turn and uncovered a potentially critical problem. The
original bug was about releasing the connection back to the pool. This
conversation is about closing the underlying socket under the same
situation. I'm not sure if we want to keep track of it under the same bug or
a new one. 

The good news is, the solution is fairly simple and Oleg lists it below. The
same code can fix Bug 25370 if we release the connection after closing the
socket.

Thanks
Moh
P.S. This is my second attempt at sending this to the list. Hope it's not a
duplicate.

-----Original Message-----
From: Oleg Kalnichevski [mailto:olegk@apache.org] 
Sent: Wednesday, December 10, 2003 4:54 PM
To: Rezaei, Mohammad A.
Subject: RE: [Bug 25370] - exception during writeRequest leaves the conn
ection un-released


Mohammad,
I do not think we should be taking this discussion off-line. I'd really
appreciate if you posted your comments to the but report. Some other folks
may have something to contribute, for instance, Mike who actually authored
Multithreaded connection manager 

On Wed, 2003-12-10 at 22:28, Rezaei, Mohammad A. wrote:
> handleException is only called when inside the WrapperOutputStream
> methods. A *read* failure while sending a file (or a runtime 
> exception, etc) will not be caught here. Am I missing something?
> 

You certainly have a point here. I guess HttpMethoDirector#executeMethod may
simply do the following

try {
 execute
}
catch (IOException e) {
  // close the damn thing
}
catch (RuntimeException e) {
  // close the damn thing
}
finally {
  // see if it is safe to release connection 
  // (no unread response content left 
  // or not response body not expected) 
}

Would you like to whip up a patch or shall I do it?

Cheers

Oleg

> Thanks
> Moh
> 
> -----Original Message-----
> From: Oleg Kalnichevski [mailto:olegk@apache.org]
> Sent: Wednesday, December 10, 2003 4:24 PM
> To: Rezaei, Mohammad A.
> Subject: Re: [Bug 25370] - exception during writeRequest leaves the conn
> ection un-released
> 
> 
> 
> On Wed, 2003-12-10 at 22:07, Rezaei, Mohammad A. wrote:
> > 9M. Reusing that socket will have very unpredictable results (it 
> > acts
> > like a black hole and won't throw any exception unless a timeout is 
> > specified). It would be prudent to close the socket on an IOException 
> > or a RuntimeException. Do you agree? Should I file this under a 
> > separate bug or under this one?
> > 
> 
> Mohammad,
> The wrapper class should take of that:
> 
>         /**
>          * Closes the connection and conditionally converts exception 
> to recoverable.
>          * @param ioe the exception that occurred
>          * @return the exception to be thrown
>          */
>         private IOException handleException(IOException ioe) {
>             // keep the original value of used, as it will be set to 
> false by close().
>             boolean isRecoverable = HttpConnection.this.used;
>             HttpConnection.this.close();
>             if (ioe instanceof InterruptedIOException) {
>                 return new IOTimeoutException(ioe.getMessage()); 
>             } else if (isRecoverable) {
>                 LOG.debug(
>                     "Output exception occurred on a used connection.
> Will treat as recoverable.", 
>                     ioe
>                 );
>                 return new HttpRecoverableException(ioe.getMessage(),
> ioe);                
>             } else {
>                 return ioe;
>             }            
>         }
> 
> Cheers,
> 
> Oleg
> 
> 
> 
> 
> > Thanks
> > Moh
> > 
> > ------- Additional Comments From olegk@apache.org  2003-12-10 20:01
> > -------
> > > if (CAUSE_RANDOM_ERROR) if (Math.random() > ERROR_RATE) throw new 
> > > IOException("Random error, for testing only!");
> > 
> > OK. I see.
> > 
> > > From reading the code, it seems the outputstream is wrapped
> > > insideWrappedOutputStream which rethrows any exceptions *inside its
> > methods*
> > > asrecoverable. There are two problems with this approach.
> > 
> > Mohammad, actually that is that way it is supposed to be. 
> > IOException
> > thrown when retrieving request content or runtime exceptions are not 
> > recoverable HTTP transport exceptions and should not be treated as 
> > such
> > 
> > > Thanks for the clarification. You still have to be extremely 
> > > careful if you enable auto recovery on write.
> > 
> > Agreed. I personally am not in favour of having auth-recovery
> > activated per default, but according to the feedback we were getting 
> > from our users the majority did not appear to share that conviction.
> > 
> > > How about this fix: add
> > >                 catch (IOException e) {
> > >                     releaseConnection = true;
> > >                     throw e;
> > >                 } catch (RuntimeException e) {
> > >                     releaseConnection = true;
> > >                     throw e;
> > >                 }
> > > to the last try clause in HttpMethodDirector#executeWithRetry ?
> > 
> > That can be done. I would not like to change 2.0 branch, though, for
> > the following reason: the invocation of HttpClient#execute(HttpMethod) 
> > MUST be followed by HttpMethod#releaseconnection regardless of the 
> > outcome (preferably by calling it in the finally clause). This is a 
> > part of HttpClient API contract. So, personally do not see this bug 
> > serious enough to warrant modification of HttpClient 2.0 which is the 
> > release candidate phase.
> > 
> > Oleg

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org


Mime
View raw message