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:[EMAIL PROTECTED] 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:[EMAIL PROTECTED] > 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 [EMAIL PROTECTED] 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
