Added fileRequestTimeout config property. #329
Conversation
marcin-cebo
requested review from
kleewho,
wkal-pubnub and
jguz-pubnub
as code owners
| @@ -31,7 +31,8 @@ class RetrofitManager( | |||
| private val configuration: PNConfiguration, | |||
| @get:TestOnly internal var transactionClientInstance: OkHttpClient? = null, | |||
| @get:TestOnly internal var subscriptionClientInstance: OkHttpClient? = null, | |||
| @get:TestOnly internal var noSignatureClientInstance: OkHttpClient? = null, | |||
| @get:TestOnly internal var noSignatureClientInstanceForFiles: OkHttpClient? = null, | |||
| @get:TestOnly internal var signatureClientInstanceForFiles: OkHttpClient? = null | |||
There was a problem hiding this comment.
is this actually used anywhere?
There was a problem hiding this comment.
It is used in test: retrofit manager created from another has shared OkHttpClients data
There was a problem hiding this comment.
so if it's not used in production code for anything, do we need it?
| * | ||
| * The value is in seconds. | ||
| * | ||
| * Defaults to 300 seconds |
There was a problem hiding this comment.
OkHttp has a good reason for defaulting this to 0 (no limit),
do we really have a reason to default this to anything else (and introduce a breaking change for users of our SDK)?
There was a problem hiding this comment.
JS has it set to 300 by default. I was using JS a reference point to keep cross SDK consistence.
I guess some changes for existing users are inevitable.
Currently users uses nonSubscribeReadTimeout to set readTimeout for Files related operation.
After change nonSubscribeReadTimeout will not be taken into account readTimeout will 0 (no limit)
and if user upgrade they will get default value for fileRequestTimeout
What do you think?
There was a problem hiding this comment.
we discussed on daily, and we don't have to have consistency because we have different (better) timeouts we can use that JS and iOS don't have access to.
We should expose connect and read timeouts for file downloads as well, and the request timeout should be 0 (no limit) by default.
| @@ -156,6 +157,7 @@ class PNConfigurationImplTest { | |||
| assertEquals(3, configuration.subscribeTimeout) | |||
| assertEquals(4, configuration.connectTimeout) | |||
| assertEquals(5, configuration.nonSubscribeRequestTimeout) | |||
| assertEquals(10, configuration.fileRequestTimeout) | |||
There was a problem hiding this comment.
unrelated, but looks like this test is missing authToken that we've added some time ago
There was a problem hiding this comment.
I will add this.
No description provided.