[Sblim-issues] [ sblim-Bugs-1984588 ] HttpClient not closed on cimclient close

[SourceForge.net <no...@so...>]

Bugs item #1984588, was opened at 2008-06-04 10:07
Message generated for change (Comment added) made by blaschke-oss
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=712784&aid=1984588&group_id=128809

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Java Client
Group: Memory Leak
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Patrick Schaefer (psnotch)
Assigned to: Dave Blaschke (blaschke-oss)
Summary: HttpClient not closed on cimclient close

Initial Comment:
The following problem appears in the SBLIM_CIM_CLIENT_1_2_6_5 branch:

On creation of a new cimclient instance and issuing any call, a new HttpClient instance is created using HttpClient.getClient(). This instance is not put into the HttpClientPool. Note that the functionality in line 192 to put it into the clientPool seemed to exist in the past.
After the call is completed, the cimclient is closed (CIMClient.close()) which later on calls HttpClientPool.closePool(). At this point all HttpClient instances contained in availableConnections are disconnected, but as the initially created HttpClient was not added to the HttpClientPool it is not in there and is not disconnected and never cleaned up.

In addition this creates problem on any CIMOM as the connection is still kept open and uses CIMOM resources.

----------------------------------------------------------------------

>Comment By: Dave Blaschke (blaschke-oss)
Date: 2008-07-30 14:37

Message:
Logged In: YES 
user_id=1215427
Originator: NO

The patch includes this code after the while loop:

  if(connection!=null) {
    connection.close();
  }

Immediately after this an exception is thrown using
connection.getResponseCode(), connection.getResponseMessage() and
connection.getHeaderField() even though the connection has been closed. 
This should result in generating a new, reset connection and retrieving
invalid information from it.  Also, why is close() used instead of
disconnect()?  close() seems to close the entire pool while disconnect()
just disconnects the one connection.  Finally, why check for a null
connection?  At this point the connection will be non-null with an HTTP
error - otherwise even the logic in the throw would cause a null pointer
exception.

What do you think about the following?

  String[] exceptStr = new String[] {
    "\"HTTP " + iConnection.getResponseCode() + 
    " " + iConnection.getResponseMessage() + "\"",
    "CIMError:\"" + iConnection.getHeaderField("CIMError") + 
    "\"" };
  connection.disconnect();
  throw new
CIMTransportException(CIMTransportException.EXT_ERR_UNABLE_TO_CONNECT, 
    exceptStr);

----------------------------------------------------------------------

Comment By: Patrick Schaefer (psnotch)
Date: 2008-07-15 01:43

Message:
Logged In: YES 
user_id=1940567
Originator: YES

Uploaded patch_1984588.txt which addresses the problem by disconnecting
every time transmitRequest() raises an exception
File Added: patch_1984588.txt

----------------------------------------------------------------------

Comment By: Patrick Schaefer (psnotch)
Date: 2008-07-14 05:51

Message:
Logged In: YES 
user_id=1940567
Originator: YES

Having a further look it seems to be ok that at creation time HttpClient
is not added to the HttpClientPool's availableConnections (as it is
obviously in use and not available).

Under normal conditions (no errors occur) the HttpClient is added to the
pool after an operation ended successfully and closed later on.

The problem can be recreated with the following scenario:
- An enumerateInstanceNames call is made but with invalid credentials.
What happens is:
- CIMClientXML.enumerateInstanceNames() issues a transmitRequest() (a
HttpUrlConnection (and internal an HttpClient) is created)
- HTTP_FORBIDDEN is received and an CIMAuthenticationException is raised
but the HttpUrlConnection is not closed. Therefore the HttpClient stays
open as well.

Shouldn't the connection be disconnected for every case transmitRequest()
throws an exception?


----------------------------------------------------------------------

Comment By: Dave Blaschke (blaschke-oss)
Date: 2008-06-05 07:31

Message:
Logged In: YES 
user_id=1215427
Originator: NO

Will have to look into it a bit more, seems it has been that way since
1.2.0:

    client = new HttpClient(url, clientPool, auth_handler);
//  allConnections.add(client);

I was just curious if you tried with the line uncommented and still found
problems...

----------------------------------------------------------------------

Comment By: Patrick Schaefer (psnotch)
Date: 2008-06-05 02:24

Message:
Logged In: YES 
user_id=1940567
Originator: YES

Please ignore the previous Canned Response "Patch pushed up to 2.x
development stream".
Seems i selected this inadvertently when adding my comment.

----------------------------------------------------------------------

Comment By: Patrick Schaefer (psnotch)
Date: 2008-06-05 01:58

Message:
Logged In: YES 
user_id=1940567
Originator: YES

Patch pushed up to 2.x development stream

----------------------------------------------------------------------

Comment By: Patrick Schaefer (psnotch)
Date: 2008-06-05 01:58

Message:
Logged In: YES 
user_id=1940567
Originator: YES

I don't know if this line was commented by intention. 
I want to make sure that we don't introduce another problem if we
uncomment it again. 
As you have more experience in the code: Do you see any problems if this
line is re-added to the source?

Thanks.

----------------------------------------------------------------------

Comment By: Dave Blaschke (blaschke-oss)
Date: 2008-06-04 16:36

Message:
Logged In: YES 
user_id=1215427
Originator: NO

Some logic that is still present in the 1.2.6.5 HttpClient was moved to
HttpClientPool in later releases:

  synchronized (clientPool) { //TODO move this code into HttpClientPool
    :
  }

So, does uncommenting line 192 in HttpClient.getClient() not fix the
problem?

  //  clientPool.addConnectionToPool(client);

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=712784&aid=1984588&group_id=128809