[android] Check for second callback in HttpRequestAndroid

[ljbade]

Fixes #2400

[ljbade]

@zugaldia @bleege @tobrun This PR seems to fix the crash. But I still not sure why the callback gets called twice. It shouldn't.

I need to dig into this more to see if I can find the actual problem here.

[ljbade]

I have squashed and tidied up the commit.

@bleege @zugaldia One last review/test will be appreciated.

Final step is to wait for CI then merge when all happy.

[bleege]

I have squashed and tidied up the commit.

Does this mean that you've found out why the callback gets called twice? If so can you explain what's going on? Thanks!

[ljbade]

@bleege I think @zugaldia's changes prevented the double callback, though I should go and double check this again with logging.

I suspect it had something to with how we handle IOExceptions in onResponse. Particularly the change from just returning on an exception, to rethrowing it.

[tobrun]

When reproducing the crash I mentioned earlier I'm differentiating 2 outcomes:

Scenario 1

the application doesn't crash, related stacktrace below

Scenario 2

the application doesn't crash but halts for a moment before continuing. (skipped frames)

[zugaldia]

To help debugging requests I just added an application interceptor (it's disabled by default): 561a36c

This interceptor does not change the request or response in any way, it simply logs the outgoing request and the incoming response data. To see it in action, you need to enable it in the HTTPContext constructor.

[ljbade]

Awesome @zugaldia.

I will also rebase this branch to bring in API 15 fixes for next SNAPSHOT

[zugaldia]

👍

[bleege]

I will also rebase this branch to bring in API 15 fixes for next SNAPSHOT

Just checking, but if we're done with this why not just merge into master? Remember, the only reason that the last SNAPSHOT was based off this branch is because we wanted to isolate and test the OkHttp work.

[ljbade]

@bleege oh true... quick question, should we run a perf benchmark somehow before we merge just to test out concerns around JNI overhead (#2400 (comment))

[bleege]

should we run a perf benchmark somehow before we merge just to test out concerns around JNI overhead

Sounds like a great idea. Please make it happen.

[manimaul]

+1 for Merging this into master. Having the networking in the Java layer could allow for implementing tile data providers in the Java layer with minimal effort. (E.g. https://github.com/manimaul/mapbox-gl-native/tree/offline)

[ljbade]

@tobrun @zugaldia Do you have some ideas on how we can benchmark the HTTP library?

There isn't an easy to request a bunch of tiles to load directly without modifying the C++ lib. Otherwise we could try panning to a set of locations after clearing the cache. We would need to add some logging or stat tracking code in the C++ layer to measure the difference however.

[ljbade]

I think I am going to merge this branch now. We can then go back and benchmark before and after once we figure out how best to do this.

[ljbade]

Ah damn the PR was marked as closed not merged!

It was merged in af2d034