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

IllegalArgumentException for the url in the Cache Entry #6453

Closed
brunoescalona-zz opened this issue Nov 30, 2020 · 42 comments
Closed

IllegalArgumentException for the url in the Cache Entry #6453

brunoescalona-zz opened this issue Nov 30, 2020 · 42 comments
Labels
bug Bug in existing code

Comments

@brunoescalona-zz
Copy link

brunoescalona-zz commented Nov 30, 2020

The following error is one of our main crashes right now, around 25% of our crashes.

Fatal Exception: java.lang.IllegalArgumentException: Expected URL scheme 'http' or 'https' but no colon was found
       at okhttp3.HttpUrl$Builder.parse$okhttp(HttpUrl.java:1260)
       at okhttp3.HttpUrl$Companion.get(HttpUrl.java:1632)
       at okhttp3.Request$Builder.url(Request.java:184)
       at okhttp3.Cache$Entry.response(Cache.java:641)
       at okhttp3.Cache.get$okhttp(Cache.java:183)
       at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.java:47)
       at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:109)
       at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.java:83)
       at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:109)
       at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.java:76)
       at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:109)
       at de.stocard.dagger.ProvidedDependenciesModule$provideHttpClient$$inlined$-addInterceptor$1.intercept(ProvidedDependenciesModule.java:1083)
       at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:109)
       at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.java:201)
       at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.java:517)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
       at java.lang.Thread.run(Thread.java:919)

The same error was reported #4322 and #4918

I would like to reopen the discussion and maybe together we can find the root cause and a solution.

During the last week we were building a custom version based on the 4.9.0 to report the url contained in the Cache Entry. We modified the following line with the code below and we also modified the cache path directory to avoid possible old corrupted files.

throw IllegalArgumentException("Expected URL scheme 'http' or 'https' but no colon was found for the url input $input")

We are getting in that case the corrupted url stored in the cache file.

For example, with the following retrofit configuration:

val clientBuilder = OkHttpClient.Builder()

clientBuilder.protocols(listOf(Protocol.HTTP_1_1))

clientBuilder.addInterceptor { chain ->
    val originalRequest = chain.request()
    val requestWithUserAgent = originalRequest.newBuilder()
            .header("User-Agent", "Example/1.0.0 HTTPClient Android").build()
    chain.proceed(requestWithUserAgent)
}

val certificatePinner = CertificatePinner.Builder()
certificatePinner.add("*.example.com", "sha256/public-key-1")
clientBuilder.certificatePinner(certificatePinner.build())

val cacheDir = File(context.cacheDir, "new_caching_directory")
val cache = Cache(cacheDir, (16 * 1024 * 1024).toLong())
clientBuilder.cache(cache)

val okhttpClient = clientBuilder.build()

val retrofit = Retrofit.Builder()
                .baseUrl("https://www.example.com/")
                .client(okHttpClient)
                .build()

val exampleApi = retrofit.create(ExampleApi::class.java)

interface ExampleApi {
    @GET("users")
    fun getUsers(): Single<List<User>>
}

We are getting in the error stack traces the following urls:

Fatal Exception: java.lang.IllegalArgumentException: Expected URL scheme 'http' or 'https' but no colon was found for the url input ��������5�����e?��뷘b����@&-�����1�W�ہ�n��?�!y6�j٬�6�N�S��https://www.example.com/users
Fatal Exception: java.lang.IllegalArgumentException: Expected URL scheme 'http' or 'https' but no colon was found for the url input ��������5T��k�;=�f�Er��,��k�H����2��C���A*�����s��������.��^https://www.example.com/users
Fatal Exception: java.lang.IllegalArgumentException: Expected URL scheme 'http' or 'https' but no colon was found for the url input ://www.example.com/users

The url stored in the cache file, as you can see, can contain some corrupted text or the https is removed from the url.

Could you have a look? I can also investigate further or give you more data if needed.

Regards

@brunoescalona-zz brunoescalona-zz added the bug Bug in existing code label Nov 30, 2020
@yschimke
Copy link
Collaborator

@brunoescalona thanks for the additional debug information.

@dave-r12
Copy link
Collaborator

Yes I'd like to get to root cause on this one too!

@brunoescalona I don't see a cache configured in your example. Is that happening elsewhere? Is it possible to provide a minimal test or code sample that you can reliably reproduce with?

@brunoescalona-zz
Copy link
Author

@dave-r12 I have updated the description with a more detailed and similar configuration. But I also can include it here below.

We can not reproduce the issue, we tried different things but it was impossible so far. After few ideas I could only simulate the behaviour of the issue "corrupting" manually the cache files and that way I was able to get the same stack trace and error, but I couldn't reproduce the root cause and understand why the cache file was corrupted.

We are also happy to provide more detailed information with few production endpoints causing the issue in a private channel if needed.

We are currently using two Okhttp instances configured as follow:

val clientBuilder = OkHttpClient.Builder()

clientBuilder.protocols(listOf(Protocol.HTTP_1_1))

clientBuilder.addInterceptor { chain ->
    val originalRequest = chain.request()
    val requestWithUserAgent = originalRequest.newBuilder()
            .header("User-Agent", "Example/1.0.0 HTTPClient Android").build()
    chain.proceed(requestWithUserAgent)
}

val certificatePinner = CertificatePinner.Builder()
certificatePinner.add("*.example.com", "sha256/public-key-1")
clientBuilder.certificatePinner(certificatePinner.build())

val cacheDir = File(context.cacheDir, "new_caching_directory")
val cache = Cache(cacheDir, (16 * 1024 * 1024).toLong())
clientBuilder.cache(cache)

val okhttpClient = clientBuilder.build()
val loggingInterceptor = HttpLoggingInterceptor()
loggingInterceptor.level = HttpLoggingInterceptor.Level.NONE

val clientBuilder = OkHttpClient.Builder()
        .protocols(listOf(Protocol.HTTP_2, Protocol.HTTP_1_1))
        .addInterceptor(loggingInterceptor)

val certificatePinner = CertificatePinner.Builder()
certificatePinner.add("*.example2.com", "sha256/public-key-2")
clientBuilder.certificatePinner(certificatePinner.build())

val httpClient = clientBuilder.build()
httpClient.dispatcher.maxRequestsPerHost = 32
httpClient.dispatcher.maxRequests = 32

val cacheDir = File(context.cacheDir, "new_caching_directory_2")
val cache = Cache(cacheDir, (8 * 1024 * 1024).toLong())
httpClient.newBuilder().cache(cache).build()

@gildor
Copy link

gildor commented Dec 2, 2020

I can confirm that we have the same issue, see it on clients with cache. And it's not an issue of request itself, we have a debug and request URL is correct, so after digging into sources, it may happen only if a cache entry has invalid value for URL

Never be able to reproduce it, only on prod. We have not so many such cases, 10-30 per day maximum, but still it in top 10 our crashes

@im-lakshya
Copy link

We've been also getting the exact same crash, and the count is now getting higher and higher. Is there any way to suppress it or to solve it?

@yschimke
Copy link
Collaborator

yschimke commented Dec 5, 2020

@im-lakshya is there any additional context here? Is it all Android versions? Any chance you have multiple clients sharing a cache?

@im-lakshya
Copy link

We're one of the libraries in which we use Picasso for image caching. And our users suspect that the crash is happening because of us and I'm not able to find out the root cause behind it, and also verified that Picasso is also internally catching the exceptions when it makes connections via OkHttp.

And the crash is happening across all the major versions (9, 10, 11) but I can see that 90% of crashes are happening in Android 10 devices. We also checked that there are multiple libraries present in the client's app which internally uses OkHttp. So we're not sure how do we ensure which library is causing this crash, or is it OkHttp doing something internally due to which it is crashing.

Let me know if you require any more information, I'll try to get them from the client.

@dave-r12
Copy link
Collaborator

dave-r12 commented Dec 6, 2020

Thanks for the info. So far, I haven't been able to spot how this could happen in practice. It seems .... impossible 😄. But there's gotta be an explanation!

This person had mentioned that when they read the cache file directly upon seeing this exception the first line was actually fine #3251 (comment). Could anyone else confirm if reading the cache file directly after seeing this exception shows a valid URL as the first line?

@yschimke
Copy link
Collaborator

yschimke commented Dec 6, 2020

Anything we could put in the next release to confirm a theory? Or recover at least enough to make this non-fatal?

} catch (re: RuntimeException) {
  throw new IOException("please report to https://github.com/square/okhttp/issues/6453 " + firstLineOfCache, re)
}

@brunoescalona-zz
Copy link
Author

@dave-r12 we were reporting the URL for each crash from here:

if (schemeDelimiterOffset != -1) {
  when {
    input.startsWith("https:", ignoreCase = true, startIndex = pos) -> {
      this.scheme = "https"
      pos += "https:".length
    }
    input.startsWith("http:", ignoreCase = true, startIndex = pos) -> {
      this.scheme = "http"
      pos += "http:".length
    }
    else -> throw IllegalArgumentException("Expected URL scheme 'http' or 'https' but was '" +
        input.substring(0, schemeDelimiterOffset) + "'")
  }
} else if (base != null) {
  this.scheme = base.scheme
} else {
  throw IllegalArgumentException("Expected URL scheme 'http' or 'https' but no colon was found for the url input $input")
}

And the urls are corrupted. I didn't go through all the code, but I was expecting to have the text url obtained from the cache file directly.

Should we also try to read the file to compare the url stored there?

@yschimke where are you planning to include that check?

@yschimke
Copy link
Collaborator

yschimke commented Dec 7, 2020

@brunoescalona not sure, I'm brainstorming what we can do in Version+1, to get information to resolve by Version+2. And wondering if we should ask for reports and also make it not fatal until then.

@gildor
Copy link

gildor commented Dec 8, 2020

We see this crash on all versions of Android, but ~70-80% are from Android 10.

And we indeed reuse cache between multiple okhttp clients. I will try to split http cache for every client (it's not really reused, it probably never request the same url, only reuses cache dir) and let you know

@swankjesse
Copy link
Member

Are you creating multiple Cache instances that all point at the same directory? That might be your problem. You want a single Cache instance that the different OkHttpClient instances all share.

@gildor
Copy link

gildor commented Dec 8, 2020

@swankjesse No, we create a single Cache instance which used by multiple clients.

@yschimke
Copy link
Collaborator

yschimke commented Dec 8, 2020

@brunoescalona an example we could check, print a noisy warning if we create two cache instances with same directory.

@hoppham2332
Copy link

I'm stuck in the same issue and I find that it occurs if you try to make a URL with more than 1 period in the domain.
Example: It's ok with http://abc.xyz/foo/ba/
But it will throw the error if domain like: http://abc.xyz.com/foo/ba/

@brunoescalona-zz
Copy link
Author

brunoescalona-zz commented Dec 29, 2020

@yschimke we included some logs when the cache object is instantiated. After checking few logs of the affected users I could see that the cache is instantiated only once.

Can we do something else to help?

Edit: by the way @yschimke we are using dagger to instantiate the cache and we are using the @singleton annotation.

@SimonMarquis
Copy link
Contributor

We've been impacted by this issue for quite a while now and after adding more logs here is what we found on our side:

We added a CacheSanitizerInterceptor that would query the Cache.urls() and evict (and log) invalid entries from the cache before processing the request.
The cache sometimes contained entries with invalid HttpUrls, more precisely, all these invalid urls we logged were all missing the same thing: their scheme. They all look like ://api.xyz.tld/path?query with the rest of the url unchanged.

HttpUrl and HttpUrl.Builder prevents having null scheme, but not an empty String. Though I didn't found any public codepath that could lead to this value since both HttpUrl.Builder() and String.toHttpUrl() prevents this.

A potential loophole would be something modifing the HttpUrl.Builder scheme property instead of the function since it's internal var scheme: String? = null.

@SimonMarquis
Copy link
Contributor

Thanks @yschimke for improving the logging in #6495 for the next release. In the meantime what could we do to progress on this issue?
If the issue is detected on the reading side, maybe we could try to detect it even earlier, meaning at the writing side ? Maybe HttpUrl.url is invalid...

@yschimke
Copy link
Collaborator

+1 to your PR, but it seems to confirm we are correctly failing already. Running HttpUrlGetTest (which extends HttpUrlTest) shows the right failure.

Expected URL scheme 'http' or 'https' but no scheme was found for ://hos...
java.lang.IllegalArgumentException: Expected URL scheme 'http' or 'https' but no scheme was found for ://hos...
	at okhttp3.HttpUrl$Builder.parse$okhttp(HttpUrl.kt:1261)
	at okhttp3.HttpUrl$Companion.get(HttpUrl.kt:1634)

@square square deleted a comment from iamen12345 Feb 27, 2021
@square square deleted a comment from iamen12345 Feb 27, 2021
yschimke pushed a commit that referenced this issue Mar 5, 2021
Test case related to cache corruption found in #6453
@Cher80
Copy link

Cher80 commented Apr 21, 2021

We have the same issue. Cant reproduce. Verison 4.9.1
image

@somayaj
Copy link

somayaj commented May 9, 2021

Can this be worked on?

@yschimke
Copy link
Collaborator

If you are willing to try OkHttp 5.0 alphas then you could put in place Cache FileSystem logging, and try to show where the problem is happening.

See https://github.com/square/okhttp/pull/6550/files

Maybe on each meaningful cache even printing the first line of the file and watching for when the corruption happens.

@danh32
Copy link

danh32 commented May 24, 2021

We have an interceptor that catches the issue and deletes the cache entry. It's not pretty, but maybe it'll help y'all as well:
https://gist.github.com/danh32/d91f938dc223bd11da2f3310b3767020

@TostF
Copy link

TostF commented Jul 19, 2021

Same exact problem and not a single clue what to look for.

@aBenVip
Copy link

aBenVip commented Sep 22, 2021

we update okHttp version to 4.9.1 , and have same issues ,

@mironov-m
Copy link

In change log for Version 5.0.0-alpha.2 I see

Fix: Fail fast when the cache is corrupted.

Is that fix for this issue? If yes how long we will wait stable 5.0.0 version? And is it possible to fix it in version 4.9.x?=)

@nicbell
Copy link

nicbell commented Nov 12, 2021

A 4.9.x fix would be great. We're getting this crash in an instance of OkHTTP internal to a 3rd party SDK so can't even add a work around. They aren't fixing it, pointing to this thread saying it is an OkHTTP problem.

@rluick15
Copy link

I see the backport was just merged #6940 to 4.9.x. Can we expect this to be released shortly? Starting to see this happening and would love to just update the library to solve but can utilize the workaround in the meantime.

@httpmurilo
Copy link

any corrections?

@topmenu-kl
Copy link

Any corrections ? Been waiting for a while now ..

@aBenVip
Copy link

aBenVip commented Jan 24, 2022

Any corrections? @yschimke

@bherbst
Copy link

bherbst commented Mar 21, 2022

The fix (#6495 and #6940) makes it much more difficult to attempt to recover from this state using something like @danh32 's CacheDeborkifier since OkHttp is catching the exception internally.

Of course a recovery mechanism built into OkHttp (#3251) would be great, but in the meantime is there another way to attempt to recover?

I suppose we could install a Logger that listens for the new "cache corruption" log, but that feels worse than the CacheDeborkifier's approach of an interceptor that catches the old exception.

For some context - of the many endpoints my app hits, only ~2 are actually cacheable and we hit those two very frequently. The cache being broken for them can mean hundreds of additional calls. I have no problem just deleting the entire cache right now if we detect it is corrupted 😆

@SimonMarquis
Copy link
Contributor

Unfortunately for us, our own sanitization step (very similar to CacheDeborkifier) did not solve most of the corruptions.
So instead, we've come up with an extra sanitization step relying on reflection 😞 (but it did solve most of the corruptions this time.)

It looks like this for each request, before calling chain.proceed(request):

  • try to get the cache entry
  • if it fails with InvocationTargetException, we try to remove the entry
private fun Cache.sanitize(request: Request) {
    kotlin.runCatching {
        invoke("get", request)
    }.onFailure {
        if (it !is InvocationTargetException) return
        kotlin.runCatching {
            invoke("remove", request)
        }
    }
}

@yoobi
Copy link

yoobi commented May 4, 2022

Hello any news about this issue ?

@NicolasGodfather
Copy link

any corrections? it happens only in production when using "minifyEnabled true" for obfuscation, and "-keep class okhttp3.**{*;}" didn't help to avoid this issue - Do any idea?

@JJSarrasin
Copy link

Hello,
Does this issue only happen with Retrofit ? I had to use the CacheDeborkifier with Retrofit and I'm doing the migration to Ktor. If yes, has someone an example to do it with the Ktor caching system ?
Thanks !

@Diper1996
Copy link

I used the OkGo library in the project, which used okhttp-3.8.1, and this error also occurred. After tracing, the error originated from line 1319 of the 'okhttp3. HttpUrl -->parse()' method.

Scenario: In the 'get' request, a 'CODED-CONTENT' returned by com. yzq. zxinglibrary. android. CaptureActivity was passed in, which is a URL. I directly passed the reference to 'CODED-CONTENT' into the 'get' method.

When 'CODED-CONTENT' is 'http://ip:port/xxx‘ This error will occur.
When 'CODED-CONTENT' is 'http://ip/xxx' When, this error will not occur.

I hope my analysis process is useful to you

@Diper1996
Copy link

I used the OkGo library in the project, which used okhttp-3.8.1, and this error also occurred. After tracing, the error originated from line 1319 of the 'okhttp3. HttpUrl -->parse()' method.

Scenario: In the 'get' request, a 'CODED-CONTENT' returned by com. yzq. zxinglibrary. android. CaptureActivity was passed in, which is a URL. I directly passed the reference to 'CODED-CONTENT' into the 'get' method.

When 'CODED-CONTENT' is 'http://ip:port/xxx‘ This error will occur. When 'CODED-CONTENT' is 'http://ip/xxx' When, this error will not occur.

I hope my analysis process is useful to you

Possible factors:
https://qa.1r1g.com/sf/ask/2492036291/
image

terms of settlement:
When I split the String object obtained by the extras. getString method and concatenate it with my own defined URL header variable, this problem will not occur.

@wahyupermadie
Copy link

is there new update related to this one ?

@Svilcata19
Copy link

I can confirm that with the newest okhttp alpha version 5.0.0-alpha.12, it's fixed - https://square.github.io/okhttp/changelogs/changelog/#version-500-alpha12 .

Please check the change log.

@swankjesse
Copy link
Member

Yep:

Fix: Recover gracefully when a cached response is corrupted on disk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in existing code
Projects
None yet
Development

No branches or pull requests