Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Http stack change via API #14

Closed
mkrasnowski opened this issue Aug 1, 2017 · 19 comments
Closed

Allow Http stack change via API #14

mkrasnowski opened this issue Aug 1, 2017 · 19 comments

Comments

@mkrasnowski
Copy link

Please add ability to change Http stack used in the implementation. HttpURLConnection is currently hardcoded.

Thanks in advance.

@Acconut
Copy link
Member

Acconut commented Aug 1, 2017

Could you please explain your situation where you want to use another HTTP implementation?

@mkrasnowski
Copy link
Author

mkrasnowski commented Aug 2, 2017

Last time I was implementing TUS on Android, HttpURLConnection did not react correctly to disabling network adapter, possibly switching it. It behaved like there was a timeout and waited a long time before failing. Once I switched to OkHttp it started to happen immediately and upload could be resumed much faster. On the other hand I prefer to use just one Http stack in my app that I have full control over.

Currently I have to add full source code of TUS to my app and change Http client myself. It would be not very hard to add this ability to library's API.

@rfelgent
Copy link

rfelgent commented Aug 3, 2017

@mkrasnowski ,

why not using "tus-android-client" instead of "tus-java-client"? I think environment specific problems are be best solved by environment specific implementations, aren't they?

@Acconut
Copy link
Member

Acconut commented Aug 3, 2017

why not using "tus-android-client" instead of "tus-java-client"? I think environment specific problems are be best solved by environment specific implementations, aren't they?

Unfortunately, this would not work since the tus-android-client is just a thin wrapper around tus-java-client and reuses most of its functionality.

Last time I was implementing TUS on Android, HttpURLConnection did not react correctly to disabling network adapter, possibly switching it. It behaved like there was a timeout and waited a long time before failing.

Could you describe your steps more detailed, please? I would like to try to reproduce this issue and attempt to see if we can fix it using the current implementation first.

It would be not very hard to add this ability to library's API.

I don't think so. Especially when considering backwards compatibility this will be very though to pull off.

@mkrasnowski
Copy link
Author

To reproduce it, just start uploading and disable network adapter. Looks like Android >= 4.4 uses OkHttp internally while older versions do not and rely on old Http stack which may be buggy. So it would make even more sense to allow for custom Http client since currently there is no way to upgrade it.

I was not able to reproduce long timeout issue this time. I don't remember, which device was affected.

Might be a good idea to support not any Http client but just OkHttp, which is extremely popular and supports HttpURLConnection interface. You just have to use this class: https://github.com/square/okhttp/blob/master/okhttp-urlconnection/src/main/java/okhttp3/internal/huc/OkHttpURLConnection.java

@rfelgent
Copy link

rfelgent commented Aug 4, 2017

I did not know that OkHttp library provides an HttpUrlConnection implementation.

In case @Acconut is willing to support this implementation I wonder if "tus-java-client" is the right place and not "tus-android-client". (see my eralier post ;-) )

@mkrasnowski
Copy link
Author

http://square.github.io/okhttp/ -> "An HTTP & HTTP/2 client for Android and Java applications"

:)

@rfelgent
Copy link

rfelgent commented Aug 6, 2017

Hi @mkrasnowski,

the OkHttp is the default HttpUrlConnection implementation since Android >= 4.4.
So why not making OkHttp as the default implementation for Android < 4.4 ? I would recommend to define a custom url protocol handler which returns an instance of OkHttp instead of JDKs default. This way you do not have to modify/adapt any code in TusUploader class!

To give you an idea have a look at this code snippet (credits: https://stackoverflow.com/questions/26363573/registering-and-using-a-custom-java-net-url-protocol):

URL.setURLStreamHandlerFactory(protocol -> "customuri".equals(protocol) ? new URLStreamHandler() {
    protected URLConnection openConnection(URL url) throws IOException {
        return new URLConnection(url) {
            public void connect() throws IOException {
                System.out.println("Connected!");
            }
        };
    }
} : null);

@mkrasnowski
Copy link
Author

@rfelgent

Wouldn't it only work with this custom protocol but not with http / https that TUS is using?

@rfelgent
Copy link

Hi @mkrasnowski ,

sorry for my late response! The code from my last response was just an example to demonstrate possibilities and put you in the right direction.

After reading some api/user guides of OkHttp I figured out, that OkHttp already provides an implementation of the URLStreamHandlerFactory.
So you only must configure it like this:

OkHttpClient okHttpClient = new OkHttpClient();    
//configure client as appropriate
//client.setXXX
URL.setURLStreamHandlerFactory(new OkUrlFactory(okHttpClient));

Be warned, that the URL.setURLStreamHandlerFactory() can only be called once (at least in Oracle JDK 8). So if some other code or library in your project calls this method, you will get an error.

The other protocols (jar, ftp and so on) should not be affected and use the JDKs default.

If you want to use the "OkUrlFactory" you must include the jar "com.squareup.okhttp3:okhttp-urlconnection", as it is not part of "com.squareup.okhttp3:okhttp"

Greetingz

@Acconut
Copy link
Member

Acconut commented Aug 16, 2017

@mkrasnowski I was able to reproduce the issues you described when disabling the network. They are, in fact, not Android-specific but are caused by Java's HttpUrlConnection implementation which is also available on desktop computers (that's how I tested it). Indeed, I could verify OkHttp does not have those problems.

Unfortunately, OkUrlFactory and OkHttpUrlConnection are deprecated (see https://github.com/square/okhttp/blob/master/okhttp-urlconnection/src/main/java/okhttp3/OkUrlFactory.java#L28-L32) meaning that we cannot use them to work around those issues.

I still don't fancy exposing an API to change the HTTP stack because this would make tus-java-client less flexible for future internal changes and refactorings.

I basically see two options left:

  1. Somehow work around the limitations of HttpUrlConnection, if possible.
  2. Replace HttpUrlConnection by OkHttp (or similar libraries).

@mkrasnowski
Copy link
Author

@Acconut Thanks for investigating the issue. Please consider option 2. OkHttp is very popular and well maintained library.

@Acconut
Copy link
Member

Acconut commented Aug 17, 2017

I will have a try but it's not as simple as one may think since OkHttp and HttpUrlConnection have some minor and larger differences in their functionality.

Of course, any help here is appreciated ;)

@TylerDuniEC
Copy link

TylerDuniEC commented Jul 5, 2018

I get an error on Android 4.1.2 API 16 that I think is related to this issue.

javax.net.ssl.SSLException: SSL handshake aborted: ssl=0xb96bc4c8: I/O error during system call, Connection reset by peer

HttpUrlConnection has a known issue on older Android versions where it will fail to negotiate the TLS/SSL connection correctly and you have to provide a custom SSLSocketFactory to fix it. Since HttpUrlConnection is hard coded in your client and can't be accessed I can't provide the fix to it. Here's a SO post discussing the issue:

https://stackoverflow.com/questions/29916962/javax-net-ssl-sslhandshakeexception-javax-net-ssl-sslprotocolexception-ssl-han

Agree with switching to OkHttp over HttpUrlConnection. Also please allow passing our own OkHttpClient to the TusClient and then use the provided OkHttpClient instead of the default one so apps can add any customizations to the OkHttpClient that particular applications need to apply to the network stack. OkHttp will also need to have a custom SSLSocketFactory applied to it on Android for example.

Update:

I was able to get around my SSL issue by setting the HttpsUrlConnection socket factory globally, running this code once on startup.

HttpsURLConnection.setDefaultSSLSocketFactory(new CustomTLSSocketFactory());

This is not ideal as it affects all HttpsUrlConnection connections and not just the TusClient's connection.

@Acconut
Copy link
Member

Acconut commented Jul 11, 2018

@TylerDuniEC Thank you for the detailed post, that helps us a lot. The move from HTTPUrlConnection to OkHttp is not a simple one as it might look for people who are not familiar with the internals of tus-java-client. I gave it a shot some time ago but wasn't able to finish it in a short time. That being said my current schedule unfortunately does not allow me to work on a move to OkHttp at the moment.
However, if you want to give it a try, I am always happy to assist you!

OkHttp will also need to have a custom SSLSocketFactory applied to it on Android for example.

Does this mean that a move to OkHttp does not solve the SSLException you mentioned unless the user injects a custom SSLSocketFactory as it's currently necessary for the HttpUrlConnection?

This is not ideal as it affects all HttpsUrlConnection connections and not just the TusClient's connection

By overwriting the TusClient#prepareConnection (https://github.com/tus/tus-java-client/blob/master/src/main/java/io/tus/java/client/TusClient.java#L274-L288) you should be able to only set the SSLSocketFactory for the connections for tus-java-client.

@TylerDuniEC
Copy link

By overwriting the TusClient#prepareConnection (https://github.com/tus/tus-java-client/blob/master/src/main/java/io/tus/java/client/TusClient.java#L274-L288) you should be able to only set the SSLSocketFactory for the connections for tus-java-client.

Thank you, overriding prepareConnection worked for me, I was able to apply the TlsSocketFactory to the Tus HttpsUrlConnection with it successfully.

@Acconut
Copy link
Member

Acconut commented Jul 18, 2018

Ok, I am glad that you were able to achieve it. I will add an entry to the README explaining this functionality for other users.

@heowc
Copy link

heowc commented Feb 24, 2022

Although this issue hasn't been discussed for a long time, I think it would be nice to provide an interface to use other http client apis besides httpUrlConnection.

Starting with Java 11, a new http client was also introduced. Other than that, there are many different http clients.

  • okhttp
  • apache-httpclient
  • armeria
  • etc...

@Acconut
Copy link
Member

Acconut commented Jan 17, 2023

Thank you for all the feedback in the past. We will be changing the HTTP client in the next major release: #78. Depending on the viability, we might also allow custom HTTP clients, but that depends on how it is to implement.

@Acconut Acconut closed this as completed Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants