Investigate and Implement shutdown() and isShutdown() for NetHttpTransport

[lqiu96]

From: googleapis/sdk-platform-java#1677 (comment)

It would be nice to be able to have a method to determine if a transport has been properly shutdown. Investigate the different clients and how to handle logic for determining if it's been shutdown or not.

[diegomarquezp]

We have shutdown as a placeholder method in HttpTransport

google-http-java-client/google-http-client/src/main/java/com/google/api/client/http/HttpTransport.java

Line 160 in 13edd13

public void shutdown() throws IOException {}

The only subclass that implements shutdown is ApacheHttpTransport:

google-http-java-client/google-http-client-apache-v2/src/main/java/com/google/api/client/http/apache/v2/ApacheHttpTransport.java

Line 211 in 13edd13

public void shutdown() throws IOException {

Does that mean we have less classes to modify (i.e. simply set isShutdown to true as default behavior of shutdown() in HttpTransport) or that we need to implement shutdown + isShutdown in all the other subclasses

cc: @suztomo

[lqiu96]

Adding more context from my recollection.

I believe we currently only implement shutdown() for both versions of the ApacheHttpTransport. Gax-HttpJson uses NetHttpTransport by default (though we do allow customization so they can use ApacheHttpTransport or another one). Gax-HttpJson currently uses a boolean value (isTransportShutdown) to determine if all the resources have been shutdown (https://github.com/googleapis/sdk-platform-java/blob/41f6ef62b96f9edd3dd594cecc5512edbc4ac4c9/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ManagedHttpJsonChannel.java#L93-L95) and this runs under assumption that no exception was thrown during the shutdown() calls. Iirc, it would be nice to replace isTransportShutdown with a call to isShutdown().

Ideally, we'd like to be have shutdown() and isShutDown() for all transports (if that is possible, but I'm not sure if there are any limitations).

[JoeWang1127]

@lqiu96 @diegomarquezp I picked up this issue in the backlog.

After briefed through all the above comments, I think the task here is to implement shutdown() for NetHttpTransport and UrlFetchTransport (two concrete, non-testing subclass of HttpTransport). Am I understanding it correctly?

[lqiu96]

@lqiu96 @diegomarquezp I picked up this issue in the backlog.

After briefed through all the above comments, I think the task here is to implement shutdown() for NetHttpTransport and UrlFetchTransport (two concrete, non-testing subclass of HttpTransport). Am I understanding it correctly?

Let me fix the title since it probably gives the wrong idea of what to try and do. The goal of this originally was to investigate how feasible it was to implement a isShutdown() implementation for all transports. That way we can use it in gax-httpjson so that we can confirm the HttpTransport has been shutdown and we can no-op when shutdown() is called multiple times.

I don't know the context as to why NetHttpTransport and UrlFetchTransport don't have a shutdown() method. Maybe there is a valid reason and we can't implement it, but that should be part of the investigation. If we can add it, we probably should.

The last part of this is to add isShutdown() for ALL of the transports if possible (again, this may not be feasible or may take a large rewrite). This issue partly investigation and if we have good reason not to implement it, then the resolution is probably something like Wont Fix.

Hope that clears it up and let me know if anything is confusing.

[JoeWang1127]

@lqiu96 Thanks for the clarification.

Though one more question, do we need to implement isShutdown() for the deprecared com.google.api.client.http.apache.ApacheHttpTransport as well?

[JoeWang1127]

Hi @lqiu96, I did some investigation on NetHttpTransport and it doesn't keep track of HttpURLConnection object in buildRequest:

google-http-java-client/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java

Lines 165 to 177 in 17bbfc1

	HttpURLConnection connection = connectionFactory.openConnection(connUrl);
	connection.setRequestMethod(method);
	// SSL settings
	if (connection instanceof HttpsURLConnection) {
	HttpsURLConnection secureConnection = (HttpsURLConnection) connection;
	if (hostnameVerifier != null) {
	secureConnection.setHostnameVerifier(hostnameVerifier);
	}
	if (sslSocketFactory != null) {
	secureConnection.setSSLSocketFactory(sslSocketFactory);
	}
	}
	return new NetHttpRequest(connection);

.

Therefore no closable resources associated with NetHttpTransport.

To implement close() on NetHttpTransport, do you think it's feasible to keep a reference of all HttpURLConnection objects that created by NetHttpTransport and call close() method in underlying InputStream or OutputStream (Java doc of HttpURLConnection)?

[lqiu96]

(Saying the below with the context that I'm not extremely familiar with how the library is written or how it works)

it doesn't keep track of HttpURLConnection object in buildRequest ... Therefore no closable resources associated with NetHttpTransport.

I wonder if there is another spot that somehow keeps track of the connection. It seems a bit odd for the library to just be opening the connections and never resource managing/ closing them. If it turns out that it is just opening the connections and never closing them, then I think we definitely need to implement closing them (potentially with the proposed solution above).

@JoeWang1127 Is this something that you can confirm? It also may be outside of the scope of this ticket/ we may need to create multiple tickets based on these findings.

[JoeWang1127]

@JoeWang1127 Is this something that you can confirm?

I'm not familiar with HttpURLConnection or NetHttpTransport either. I have this idea by reading the java doc. It seems that nothing is closable in NetHttpTransport so I dig into its superclass.

[lqiu96]

I took a quick look and I believe I the NetHttpRequest has a connection.disconnect() call:

google-http-java-client/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java

Lines 153 to 155 in 17bbfc1

	if (!successfulConnection) {
	connection.disconnect();
	}

It may be possible store a list of the connections that have been opened. Might have some unintended side effects... Off the top of my head (which may or may not end up being a concern): If most connections are closed properly, we may end up holding a list of closed connections that just exist in the list for long running processes.

[suztomo]

Avoid keeping unnecessary resources open.

[JoeWang1127]

After discuss with @blakeli0, this issue is limited to implement shutdown and isShutdown for NetHttpTransport since it's the default http transport used in gax.

[JoeWang1127]

I thought building request (but not sending it) might be a source of memory leak. However, it seems not true.

I create a project to build get request in an infinite loop and the memory usage is stable: