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.

Sign up for GitHub

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

Added fileRequestTimeout config property. #329

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added fileRequestTimeout config property. #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions kotlin-js-store/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,10 +659,10 @@ proxy-from-env@^1.1.0:
resolved "https://registry.yarnpkg.com/proxy-from-env/-/proxy-from-env-1.1.0.tgz#e102f16ca355424865755d2c9e8ea4f24d58c3e2"
integrity sha512-D+zkORCbA9f1tdWRK0RaCR3GPv50cMxcrz4X8k5LTSUD1Dkw47mKJEZQNunItRTkWwgtaUSo1RVFRIG9ZXiFYg==

[email protected].0:
version "8.4.0"
resolved "https://registry.yarnpkg.com/pubnub/-/pubnub-8.4.0.tgz#8a5fdd3af9783abb1de44ea4145107e296c8394d"
integrity sha512-HvkFhn4XcfR1wdJv4paX94I0TT4UBHW/vIuYnS+6XuoSx0AunJMCk5wbKnSesmdbzYUZa8Ag3H1mJQZKuyRy8Q==
[email protected].1:
version "8.4.1"
resolved "https://registry.yarnpkg.com/pubnub/-/pubnub-8.4.1.tgz#5f6f19e84d3187dc8aee0a458bd6b05e90d43e6a"
integrity sha512-mPlwVoHJDWPasZx52UfSMiPX5TATm5A+ficSogyqDqTQ4w5EQnwxH+PJdsWc0mPnlT051jM1vIISMeM0fQ30CQ==
dependencies:
agentkeepalive "^3.5.2"
buffer "^6.0.3"
Expand Down
24 changes: 24 additions & 0 deletions pubnub-gson/pubnub-gson-api/src/main/java/com/pubnub/api/java/v2/PNConfiguration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,17 @@ interface PNConfiguration : com.pubnub.api.v2.PNConfiguration {
*/
val nonSubscribeReadTimeout: Int

/**
* Timeout for file-related operations (e.g. sendFile, listChannelFiles, deleteFile, etc.)
* This timeout applies to the entire call which includes: resolving DNS, connecting, writing the request body,
* server processing and reading the response body.
*
* The value is in seconds.
*
* Defaults to 300 seconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@marcin-cebo marcin-cebo Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@wkal-pubnub wkal-pubnub Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
val fileRequestTimeout: Int

/**
* If operating behind a misbehaving proxy, allow the client to shuffle the subdomains.
*
Expand Down Expand Up @@ -394,6 +405,8 @@ interface PNConfiguration : com.pubnub.api.v2.PNConfiguration {

override fun nonSubscribeReadTimeout(nonSubscribeReadTimeout: Int): Builder

override fun fileRequestTimeout(fileRequestTimeout: Int): Builder

/**
* If operating behind a misbehaving proxy, allow the client to shuffle the subdomains.
*
Expand Down Expand Up @@ -624,6 +637,17 @@ interface PNConfigurationOverride : com.pubnub.api.v2.PNConfigurationOverride {
*/
fun nonSubscribeReadTimeout(nonSubscribeReadTimeout: Int): Builder

/**
* Timeout for file-related operations (e.g. sendFile, listChannelFiles, deleteFile, etc.)
* This timeout applies to the entire call which includes: resolving DNS, connecting, writing the request body,
* server processing and reading the response body.
*
* The value is in seconds.
*
* Defaults to 300 seconds
*/
fun fileRequestTimeout(fileRequestTimeout: Int): Builder

/**
* Create a [PNConfiguration] object with values from this builder.
*/
Expand Down
10 changes: 10 additions & 0 deletions ...ub-gson/pubnub-gson-impl/src/main/java/com/pubnub/internal/java/v2/PNConfigurationImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class PNConfigurationImpl(
override val subscribeTimeout: Int = SUBSCRIBE_TIMEOUT,
override val connectTimeout: Int = CONNECT_TIMEOUT,
override val nonSubscribeReadTimeout: Int = NON_SUBSCRIBE_REQUEST_TIMEOUT,
override val fileRequestTimeout: Int = FILE_REQUEST_TIMEOUT,
override val cacheBusting: Boolean = false,
override val suppressLeaveEvents: Boolean = false,
override val maintainPresenceState: Boolean = true,
Expand Down Expand Up @@ -79,6 +80,7 @@ class PNConfigurationImpl(
const val NON_SUBSCRIBE_REQUEST_TIMEOUT = 10
const val SUBSCRIBE_TIMEOUT = 310
const val CONNECT_TIMEOUT = 5
const val FILE_REQUEST_TIMEOUT = 300
}

class Builder internal constructor(defaultConfiguration: com.pubnub.api.v2.PNConfiguration) :
Expand Down Expand Up @@ -207,6 +209,13 @@ class PNConfigurationImpl(

override var nonSubscribeReadTimeout: Int = defaultConfiguration.nonSubscribeReadTimeout

override fun fileRequestTimeout(fileRequestTimeout: Int): Builder {
this.fileRequestTimeout = fileRequestTimeout
return this
}

override var fileRequestTimeout: Int = defaultConfiguration.fileRequestTimeout

override fun cacheBusting(cacheBusting: Boolean): Builder {
this.cacheBusting = cacheBusting
return this
Expand Down Expand Up @@ -385,6 +394,7 @@ class PNConfigurationImpl(
subscribeTimeout = subscribeTimeout,
connectTimeout = connectTimeout,
nonSubscribeReadTimeout = nonSubscribeReadTimeout,
fileRequestTimeout = fileRequestTimeout,
cacheBusting = cacheBusting,
suppressLeaveEvents = suppressLeaveEvents,
maintainPresenceState = maintainPresenceState,
Expand Down
3 changes: 3 additions & 0 deletions ...n/pubnub-gson-impl/src/test/kotlin/com/pubnub/internal/java/v2/PNConfigurationImplTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class PNConfigurationImplTest {
.subscribeTimeout(3)
.connectTimeout(4)
.nonSubscribeReadTimeout(5)
.fileRequestTimeout(10)
.cacheBusting(true)
.suppressLeaveEvents(true)
.maintainPresenceState(false)
Expand Down Expand Up @@ -124,6 +125,7 @@ class PNConfigurationImplTest {
assertEquals(3, configuration.subscribeTimeout)
assertEquals(4, configuration.connectTimeout)
assertEquals(5, configuration.nonSubscribeReadTimeout)
assertEquals(10, configuration.fileRequestTimeout)
assertEquals(true, configuration.cacheBusting)
assertEquals(true, configuration.suppressLeaveEvents)
assertEquals(false, configuration.maintainPresenceState)
Expand Down Expand Up @@ -170,6 +172,7 @@ class PNConfigurationImplTest {
assertEquals(expectedDefaults.subscribeTimeout, builder.subscribeTimeout)
assertEquals(expectedDefaults.connectTimeout, builder.connectTimeout)
assertEquals(expectedDefaults.nonSubscribeReadTimeout, builder.nonSubscribeReadTimeout)
assertEquals(expectedDefaults.fileRequestTimeout, builder.fileRequestTimeout)
assertEquals(expectedDefaults.cacheBusting, builder.cacheBusting)
assertEquals(expectedDefaults.suppressLeaveEvents, builder.suppressLeaveEvents)
assertEquals(expectedDefaults.maintainPresenceState, builder.maintainPresenceState)
Expand Down
33 changes: 33 additions & 0 deletions pubnub-kotlin/pubnub-kotlin-core-api/src/jvmMain/kotlin/com/pubnub/api/v2/PNConfiguration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,17 @@ actual interface PNConfiguration {
*/
val nonSubscribeReadTimeout: Int

/**
* Timeout for file-related operations (e.g. sendFile, listChannelFiles, deleteFile, etc.)
* This timeout applies to the entire call which includes: resolving DNS, connecting, writing the request body,
* server processing and reading the response body.
*
* The value is in seconds.
*
* Defaults to 300 seconds
*/
val fileRequestTimeout: Int

/**
* If operating behind a misbehaving proxy, allow the client to shuffle the subdomains.
*
Expand Down Expand Up @@ -456,6 +467,17 @@ actual interface PNConfiguration {
*/
var nonSubscribeReadTimeout: Int

/**
* Timeout for file-related operations (e.g. sendFile, listChannelFiles, deleteFile, etc.)
* This timeout applies to the entire call which includes: resolving DNS, connecting, writing the request body,
* server processing and reading the response body.
*
* The value is in seconds.
*
* Defaults to 300 seconds
*/
var fileRequestTimeout: Int

/**
* If operating behind a misbehaving proxy, allow the client to shuffle the subdomains.
*
Expand Down Expand Up @@ -701,6 +723,17 @@ interface PNConfigurationOverride {
*/
var nonSubscribeReadTimeout: Int

/**
* Timeout for file-related operations (e.g. sendFile, listChannelFiles, deleteFile, etc.)
* This timeout applies to the entire call which includes: resolving DNS, connecting, writing the request body,
* server processing and reading the response body.
*
* The value is in seconds.
*
* Defaults to 300 seconds
*/
val fileRequestTimeout: Int

/**
* Create a [PNConfiguration] object with values from this builder.
*/
Expand Down
25 changes: 15 additions & 10 deletions ...kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/managers/RetrofitManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this actually used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in test: retrofit manager created from another has shared OkHttpClients data

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if it's not used in production code for anything, do we need it?

) {
private var signatureInterceptor: SignatureInterceptor = SignatureInterceptor(configuration)

Expand All @@ -58,20 +59,21 @@ class RetrofitManager(
configuration,
retrofitManager.transactionClientInstance,
retrofitManager.subscriptionClientInstance,
retrofitManager.noSignatureClientInstance,
retrofitManager.noSignatureClientInstanceForFiles,
retrofitManager.signatureClientInstanceForFiles
)

init {
if (!configuration.googleAppEngineNetworking) {
transactionClientInstance = createOkHttpClient(configuration.nonSubscribeReadTimeout, parentOkHttpClient = transactionClientInstance)
subscriptionClientInstance = createOkHttpClient(configuration.subscribeTimeout, parentOkHttpClient = subscriptionClientInstance)
noSignatureClientInstance =
createOkHttpClient(configuration.nonSubscribeReadTimeout, withSignature = false, parentOkHttpClient = noSignatureClientInstance)
transactionClientInstance = createOkHttpClient(readTimeout = configuration.nonSubscribeReadTimeout, parentOkHttpClient = transactionClientInstance)
subscriptionClientInstance = createOkHttpClient(readTimeout = configuration.subscribeTimeout, parentOkHttpClient = subscriptionClientInstance)
noSignatureClientInstanceForFiles = createOkHttpClient(callTimeout = configuration.fileRequestTimeout, withSignature = false, parentOkHttpClient = noSignatureClientInstanceForFiles)
signatureClientInstanceForFiles = createOkHttpClient(callTimeout = configuration.fileRequestTimeout, parentOkHttpClient = signatureClientInstanceForFiles)
}

val transactionInstance = createRetrofit(transactionClientInstance)
val subscriptionInstance = createRetrofit(subscriptionClientInstance)
val noSignatureInstance = createRetrofit(noSignatureClientInstance)
val noSignatureInstance = createRetrofit(noSignatureClientInstanceForFiles)

timeService = transactionInstance.create(TimeService::class.java)
publishService = transactionInstance.create(PublishService::class.java)
Expand All @@ -94,16 +96,18 @@ class RetrofitManager(
}

private fun createOkHttpClient(
readTimeout: Int,
readTimeout: Int = 0, // 0 means no timeout,
callTimeout: Int = 0, // 0 means no timeout,
withSignature: Boolean = true,
parentOkHttpClient: OkHttpClient? = null,
parentOkHttpClient: OkHttpClient? = null
): OkHttpClient {
val okHttpBuilder = parentOkHttpClient?.newBuilder() ?: OkHttpClient.Builder()

okHttpBuilder
.retryOnConnectionFailure(false)
.readTimeout(readTimeout.toLong(), TimeUnit.SECONDS)
.connectTimeout(configuration.connectTimeout.toLong(), TimeUnit.SECONDS)
.callTimeout(callTimeout.toLong(), TimeUnit.SECONDS)

with(configuration) {
if (logVerbosity == PNLogVerbosity.BODY) {
Expand Down Expand Up @@ -163,7 +167,8 @@ class RetrofitManager(
fun destroy(force: Boolean = false) {
closeExecutor(transactionClientInstance, force)
closeExecutor(subscriptionClientInstance, force)
closeExecutor(noSignatureClientInstance, force)
closeExecutor(noSignatureClientInstanceForFiles, force)
closeExecutor(signatureClientInstanceForFiles, force)
}

private fun closeExecutor(
Expand Down
5 changes: 5 additions & 0 deletions ...b-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/v2/PNConfigurationImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class PNConfigurationImpl(
override val subscribeTimeout: Int = SUBSCRIBE_TIMEOUT,
override val connectTimeout: Int = CONNECT_TIMEOUT,
override val nonSubscribeReadTimeout: Int = NON_SUBSCRIBE_REQUEST_TIMEOUT,
override val fileRequestTimeout: Int = FILE_REQUEST_TIMEOUT,
override val cacheBusting: Boolean = false,
override val suppressLeaveEvents: Boolean = false,
override val maintainPresenceState: Boolean = true,
Expand Down Expand Up @@ -79,6 +80,7 @@ class PNConfigurationImpl(
const val NON_SUBSCRIBE_REQUEST_TIMEOUT = 10
const val SUBSCRIBE_TIMEOUT = 310
const val CONNECT_TIMEOUT = 5
const val FILE_REQUEST_TIMEOUT = 300
}

class Builder(defaultConfiguration: PNConfiguration) :
Expand Down Expand Up @@ -140,6 +142,8 @@ class PNConfigurationImpl(

override var nonSubscribeReadTimeout: Int = defaultConfiguration.nonSubscribeReadTimeout

override var fileRequestTimeout: Int = defaultConfiguration.fileRequestTimeout

override var cacheBusting: Boolean = defaultConfiguration.cacheBusting

override var suppressLeaveEvents: Boolean = defaultConfiguration.suppressLeaveEvents
Expand Down Expand Up @@ -201,6 +205,7 @@ class PNConfigurationImpl(
subscribeTimeout = subscribeTimeout,
connectTimeout = connectTimeout,
nonSubscribeReadTimeout = nonSubscribeReadTimeout,
fileRequestTimeout = fileRequestTimeout,
cacheBusting = cacheBusting,
suppressLeaveEvents = suppressLeaveEvents,
maintainPresenceState = maintainPresenceState,
Expand Down
2 changes: 2 additions & 0 deletions pubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/legacy/PubNubImplTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,14 @@ class PubNubImplTest : BaseTest() {
config.connectTimeout = 4000
config.nonSubscribeRequestTimeout = 5000
config.retryConfiguration = RetryConfiguration.None
config.fileRequestTimeout = 400
// initPubNub()

assertEquals("https://ps.pndsn.com", pubnub.baseUrl())
assertEquals(3000, config.subscribeTimeout)
assertEquals(4000, config.connectTimeout)
assertEquals(5000, config.nonSubscribeRequestTimeout)
assertEquals(400, config.fileRequestTimeout)
}

@Test
Expand Down
16 changes: 12 additions & 4 deletions ...in/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/internal/managers/RetrofitManagerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ class RetrofitManagerTest : BaseTest() {
clonedRetrofitManager.transactionClientInstance!!.dispatcher,
)
Assert.assertEquals(
retrofitManager.noSignatureClientInstance!!.dispatcher,
clonedRetrofitManager.noSignatureClientInstance!!.dispatcher,
retrofitManager.noSignatureClientInstanceForFiles!!.dispatcher,
clonedRetrofitManager.noSignatureClientInstanceForFiles!!.dispatcher,
)
Assert.assertEquals(
retrofitManager.signatureClientInstanceForFiles!!.dispatcher,
clonedRetrofitManager.signatureClientInstanceForFiles!!.dispatcher
)

Assert.assertEquals(
Expand All @@ -34,8 +38,12 @@ class RetrofitManagerTest : BaseTest() {
clonedRetrofitManager.transactionClientInstance!!.connectionPool,
)
Assert.assertEquals(
retrofitManager.noSignatureClientInstance!!.connectionPool,
clonedRetrofitManager.noSignatureClientInstance!!.connectionPool,
retrofitManager.noSignatureClientInstanceForFiles!!.connectionPool,
clonedRetrofitManager.noSignatureClientInstanceForFiles!!.connectionPool,
)
Assert.assertEquals(
retrofitManager.signatureClientInstanceForFiles!!.connectionPool,
clonedRetrofitManager.signatureClientInstanceForFiles!!.connectionPool
)
}

Expand Down
3 changes: 3 additions & 0 deletions ...tlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/internal/v2/PNConfigurationImplTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ class PNConfigurationImplTest {
subscribeTimeout = 3
connectTimeout = 4
nonSubscribeRequestTimeout = 5
fileRequestTimeout = 10
cacheBusting = true
suppressLeaveEvents = true
maintainPresenceState = false
Expand Down Expand Up @@ -156,6 +157,7 @@ class PNConfigurationImplTest {
assertEquals(3, configuration.subscribeTimeout)
assertEquals(4, configuration.connectTimeout)
assertEquals(5, configuration.nonSubscribeRequestTimeout)
assertEquals(10, configuration.fileRequestTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated, but looks like this test is missing authToken that we've added some time ago

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add this.

assertEquals(true, configuration.cacheBusting)
assertEquals(true, configuration.suppressLeaveEvents)
assertEquals(false, configuration.maintainPresenceState)
Expand Down Expand Up @@ -202,6 +204,7 @@ class PNConfigurationImplTest {
assertEquals(expectedDefaults.subscribeTimeout, builder.subscribeTimeout)
assertEquals(expectedDefaults.connectTimeout, builder.connectTimeout)
assertEquals(expectedDefaults.nonSubscribeRequestTimeout, builder.nonSubscribeRequestTimeout)
assertEquals(expectedDefaults.fileRequestTimeout, builder.fileRequestTimeout)
assertEquals(expectedDefaults.cacheBusting, builder.cacheBusting)
assertEquals(expectedDefaults.suppressLeaveEvents, builder.suppressLeaveEvents)
assertEquals(expectedDefaults.maintainPresenceState, builder.maintainPresenceState)
Expand Down
Loading