readrops / Readrops

Android multi-services RSS client
GNU General Public License v3.0
272 stars 20 forks source link

Having only one OkHttpClient with a globally changeable AuthInterceptor is a bad idea #93

Open shunf4 opened 3 years ago

shunf4 commented 3 years ago

This has caused what was described in #84 but it is relatively mild.

But think of the situation: when you are switching accounts in MainActivity, SyncWorker starts to sync! This is terrible.

(I guess this situation occurs more frequently than we think; for example when Readrops launches after it is previously killed)

I believe this has caused FOREIGN KEY constraint failed errors to happen and feeds to be deleted (along with its items) (because wrong accountFeedIds or accountFolderIds was generated) for several times on my device.

I think we should get rid of the single globally changeable AuthInterceptor (at least it should not be changed every time an ARepository is created ).

Shinokuni commented 3 years ago

Hello,

I created AuthInterceptor to deal with the fact that you can't modify a Retrofit instance url. Currently in Readrops, each account type has a single Retrofit instance which is reused when switching account. But as you can't modify those objects' url, the Retrofit/OkHttp team suggested as a solution to set the url a the OkHttp level, with an interceptor. Moreover, as previously said, having more than one OkHttp instance is a bad practice as creating an instance is very expensive.

It's true that I pushed AuthInterceptor without fixing all related bugs, by lack of time. I wasn't aware of all the issues you mentionned, a bit annoying indeed.

What we could do is to create an instance of Retrofit each time a ARepository object is created. This way, no need to set the url and use a already existing Retrofit object, we would only need to set the credentials which still need to be added at the http level with a header. This shouldn't be too expensive.