2 MB message support#397
Closed
marcin-cebo wants to merge 19 commits into
Closed
Conversation
marcin-cebo
requested review from
jakub-grzesiowski,
jguz-pubnub,
kleewho and
parfeon
as code owners
jguz-pubnub
approved these changes
Feb 18, 2026
marcin-cebo
force-pushed
the
2MB_message
branch
from
de6da0a to
f282673
Compare
…ssages client gets socket close instead of 413 error. I tried with “Expect: 100-continue” header that works for oversize messages but for regular messages e.g. 100KB causes server not sending 100 continue status code resulting in client waiting till timeout and not sending request body to server.
Fixed logging interceptor no longer consume the response body, so server error now properly reach CoreEndpoint
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds size-aware PublishEndpoint routing (choose V1 GET/POST or V2 POST by measured request size), a MESSAGE_TOO_LARGE error, non-consuming HTTP response logging, a new PublishService V2 POST, helper/test updates, and unit/integration tests validating routing, signature overhead, and client-side oversize rejection. ChangesLarge-message publish with dynamic V1/V2 routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 130 |
| Duplication | 54 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pubnub-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/endpoints/pubsub/PublishEndpoint.kt (1)
128-163: ⚖️ Poor tradeoffConsider: V1 GET Call is built but potentially discarded.
The code builds
v1GetCall(line 130) to calculate path length, but if routing to V2, that call is discarded. While this is acceptable for code clarity and the performance impact is likely negligible, consider whether lazy evaluation or path-length estimation without call construction would be cleaner. Not blocking—the current approach is straightforward and maintainable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubnub-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/endpoints/pubsub/PublishEndpoint.kt` around lines 128 - 163, The code currently constructs v1GetCall via retrofitManager.publishService.publish(...) just to measure path length and then discards it when switching to V2; avoid this waste by either making v1GetCall lazy or by changing the path-length calculation to operate on the message string and query components instead of an HTTP call. Specifically, stop eagerly calling publishService.publish(...) in PublishEndpoint.kt: use getParamMessage(message) together with queryParams and configuration to estimate the GET path length (or change calculateV1GetPathLengthBytes to accept the stringified message and params), and only build/return v1GetCall when the path fits; keep the existing V2 flow that uses getBodyMessage(message), calculateV1PostBodySizeBytes(payload) and enforceV2PostBodySizeLimit before calling publishWithPostV2.pubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/endpoints/pubsub/PublishV2RoutingTest.kt (1)
75-75: ⚡ Quick winReplace empty function blocks to clear detekt warnings.
Use expression bodies (
= Unit) for no-op overrides to resolveEmptyFunctionBlockwarnings cleanly.Suggested patch
- override fun enqueue(callback: Callback<List<Any>>) {} + override fun enqueue(callback: Callback<List<Any>>) = Unit @@ - override fun cancel() {} + override fun cancel() = Unit @@ - override fun enqueue(callback: Callback<List<Any>>) {} + override fun enqueue(callback: Callback<List<Any>>) = Unit @@ - override fun cancel() {} + override fun cancel() = UnitAlso applies to: 79-79, 93-93, 97-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/endpoints/pubsub/PublishV2RoutingTest.kt` at line 75, The no-op override methods (e.g., override fun enqueue(callback: Callback<List<Any>>) {}) cause Detekt EmptyFunctionBlock warnings; replace these empty function blocks with expression bodies using = Unit for each no-op override (including the occurrences at the other reported lines) so the overrides become concise expression-bodied functions (refer to the enqueue(callback: Callback<List<Any>>) override and other similar no-op overrides in PublishV2RoutingTest).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@pubnub-kotlin/pubnub-kotlin-core-api/src/commonMain/kotlin/com/pubnub/api/PubNubError.kt`:
- Around line 253-256: Add a note to the release/changelog documenting that a
new public enum entry PubNubError.MESSAGE_TOO_LARGE (code 184) was introduced
and may break client code using exhaustive Kotlin when-expressions without an
else branch; explicitly instruct consumers to update their when blocks to handle
PubNubError.MESSAGE_TOO_LARGE or add an else/default branch, and reference the
PubNubError enum in the release notes so downstream users are aware of the
source-compatibility change.
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/logging/networkLogging/CustomPnHttpLoggingInterceptor.kt`:
- Around line 94-113: The body-peeking logic in CustomPnHttpLoggingInterceptor
unconditionally buffers/clones the entire response and Base64-encodes non-text
bodies, causing heavy allocations on large responses; change the interceptor to
first check the configured logVerbosity/verbosity level (or a shouldLogBody
flag) before calling responseBody.source(), source.request(...), buffer.clone(),
buffer.readByteArray(), or Base64.getEncoder() so that buffering and encoding
only occur when body logging is enabled (keep the existing error handling and
UTF-8 fallback and preserve response readability when you do buffer by using the
same clone/read approach).
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/endpoints/pubsub/PublishV2RoutingTest.kt`:
- Around line 70-73: The test doubles FakeCallWithUrl and FakeCall violate
Retrofit's Call.clone() contract by returning the same instance; update their
clone() implementations to return a new independent Call instance (e.g., create
and return a new FakeCallWithUrl(url) for FakeCallWithUrl and a new
FakeCall(...) for FakeCall), preserving any constructor state (like url) so
retries/executions behave independently.
---
Nitpick comments:
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/endpoints/pubsub/PublishEndpoint.kt`:
- Around line 128-163: The code currently constructs v1GetCall via
retrofitManager.publishService.publish(...) just to measure path length and then
discards it when switching to V2; avoid this waste by either making v1GetCall
lazy or by changing the path-length calculation to operate on the message string
and query components instead of an HTTP call. Specifically, stop eagerly calling
publishService.publish(...) in PublishEndpoint.kt: use getParamMessage(message)
together with queryParams and configuration to estimate the GET path length (or
change calculateV1GetPathLengthBytes to accept the stringified message and
params), and only build/return v1GetCall when the path fits; keep the existing
V2 flow that uses getBodyMessage(message), calculateV1PostBodySizeBytes(payload)
and enforceV2PostBodySizeLimit before calling publishWithPostV2.
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/endpoints/pubsub/PublishV2RoutingTest.kt`:
- Line 75: The no-op override methods (e.g., override fun enqueue(callback:
Callback<List<Any>>) {}) cause Detekt EmptyFunctionBlock warnings; replace these
empty function blocks with expression bodies using = Unit for each no-op
override (including the occurrences at the other reported lines) so the
overrides become concise expression-bodied functions (refer to the
enqueue(callback: Callback<List<Any>>) override and other similar no-op
overrides in PublishV2RoutingTest).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de259f5b-4800-484b-85d0-ab004b2bf4be
📒 Files selected for processing (10)
pubnub-gson/pubnub-gson-impl/src/integrationTest/java/com/pubnub/api/integration/PublishIntegrationTests.javapubnub-kotlin/pubnub-kotlin-core-api/src/commonMain/kotlin/com/pubnub/api/PubNubError.ktpubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/PublishIntegrationTests.ktpubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/test/Extensions.ktpubnub-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/endpoints/pubsub/PublishEndpoint.ktpubnub-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/logging/networkLogging/CustomPnHttpLoggingInterceptor.ktpubnub-kotlin/pubnub-kotlin-impl/src/main/kotlin/com/pubnub/internal/services/PublishService.ktpubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/endpoints/pubsub/PublishTest.ktpubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/endpoints/pubsub/PublishV2RoutingTest.ktpubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/internal/suite/pubsub/PublishV2PostTestSuite.kt
Comment on lines
+253
to
256
| MESSAGE_TOO_LARGE( | ||
| code = 184, | ||
| message = "Message payload exceeds the maximum allowed size of 2 MiB.", | ||
| ) |
There was a problem hiding this comment.
Flag: New enum entry may break exhaustive when-expressions.
Adding MESSAGE_TOO_LARGE to the public PubNubError enum is a potential source-compatibility break for customer code with exhaustive when expressions (no else branch). Ensure release notes document this addition and advise customers to handle the new error or add an else branch.
As per coding guidelines, "Flag any addition, removal, rename, or reordering of entries in public enums (RetryableEndpointGroup, PNLogVerbosity, PNHeartbeatNotificationOptions) and sealed class hierarchies (RetryConfiguration variants) as a potential source-compatibility break for customers with exhaustive when-expressions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@pubnub-kotlin/pubnub-kotlin-core-api/src/commonMain/kotlin/com/pubnub/api/PubNubError.kt`
around lines 253 - 256, Add a note to the release/changelog documenting that a
new public enum entry PubNubError.MESSAGE_TOO_LARGE (code 184) was introduced
and may break client code using exhaustive Kotlin when-expressions without an
else branch; explicitly instruct consumers to update their when blocks to handle
PubNubError.MESSAGE_TOO_LARGE or add an else/default branch, and reference the
PubNubError enum in the release notes so downstream users are aware of the
source-compatibility change.
Comment on lines
+70
to
+73
| class FakeCallWithUrl(private val url: String) : Call<List<Any>> { | ||
| override fun clone(): Call<List<Any>> = this | ||
|
|
||
| override fun execute(): Response<List<Any>> = Response.success(listOf("test1", "test2", "10")) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Retrofit 2, what is the expected behavior of retrofit2.Call.clone()for customCall implementations, and should it return a new call instance?
💡 Result:
In Retrofit 2, retrofit2.Call.clone is expected to create a new, identical Call instance that can be executed or enqueued even if the original Call has already been executed/enqueued [1][2]. In other words, clone should not return the same (already-used/busy) Call instance; it should return a fresh Call object that represents the same request parameters and can be used independently (e.g., for retries or polling) [1][2]. For custom Call implementations, implement clone to return a new Call that is “functionally the same” but independent in execution state (i.e., it can be enqueued/executed separately from the original). This typically means your clone should return a new wrapper Call instance and also clone any delegate Call it wraps, so that the delegate’s execution state is not shared [3]. So yes: clone should return a new call instance for custom Call implementations, not the same instance, per the contract “Create a new, identical call … which can be enqueued or executed even if this call has already been” [1][2].
Citations:
- 1: abstract class implementation Call<T> square/retrofit#1059
- 2: https://blog.christophe-henry.dev/2020/02/23/tweak-your-retrofit-behavior-with-annotations.html
- 3: https://github.com/square/retrofit/blob/b3ea768567e9e1fb1ba987bea021dbc0ead4acd4/retrofit/src/main/java/retrofit2/Call.java
Return a new Call instance from clone() in test doubles.
Retrofit's Call.clone() contract requires returning a new, independent Call instance—not the same instance. Both FakeCallWithUrl and FakeCall currently return this, which violates the contract and can mask retry/re-execution behavior in tests.
Suggested patch
class FakeCallWithUrl(private val url: String) : Call<List<Any>> {
- override fun clone(): Call<List<Any>> = this
+ override fun clone(): Call<List<Any>> = FakeCallWithUrl(url)
@@
class FakeCall : Call<List<Any>> {
- override fun clone(): Call<List<Any>> = this
+ override fun clone(): Call<List<Any>> = FakeCall()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class FakeCallWithUrl(private val url: String) : Call<List<Any>> { | |
| override fun clone(): Call<List<Any>> = this | |
| override fun execute(): Response<List<Any>> = Response.success(listOf("test1", "test2", "10")) | |
| class FakeCallWithUrl(private val url: String) : Call<List<Any>> { | |
| override fun clone(): Call<List<Any>> = FakeCallWithUrl(url) | |
| override fun execute(): Response<List<Any>> = Response.success(listOf("test1", "test2", "10")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/endpoints/pubsub/PublishV2RoutingTest.kt`
around lines 70 - 73, The test doubles FakeCallWithUrl and FakeCall violate
Retrofit's Call.clone() contract by returning the same instance; update their
clone() implementations to return a new independent Call instance (e.g., create
and return a new FakeCallWithUrl(url) for FakeCallWithUrl and a new
FakeCall(...) for FakeCall), preserving any constructor state (like url) so
retries/executions behave independently.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@pubnub-gson/pubnub-gson-impl/src/integrationTest/java/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.java`:
- Around line 113-136: The test adds an anonymous SubscribeCallback via
pubNub.addListener(...) but never removes it and also leaves an extra PubNub
client un-shutdown later; to fix, capture the SubscribeCallback instance in a
local variable (instead of anonymous), call pubNub.removeListener(theCallback)
after the Awaitility.await completes (or in a finally/teardown block), and
ensure any additional PubNub client created (the temporary client variable used
around lines ~233–245) is properly shutdown by calling its shutdown() (or
closing it in teardown) to avoid cross-test listener/resource leaks.
- Line 94: In LargeMessagePublishIntegrationTests replace the two fixed pauses
(calls to pause(3)) with condition-based waits: remove pause(3) and instead
poll/await until the expected post-publish condition is true (e.g., publish
completion/receipt, message present on subscriber, or a PubNub ack) with a
reasonable timeout and fail-on-timeout; locate the pause calls in the test
methods inside class LargeMessagePublishIntegrationTests and use the test
harness's existing wait/poll utilities (or implement a simple loop with sleep +
timeout) to check the specific condition that the test asserts before
proceeding.
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.kt`:
- Around line 196-199: The test awaiting near-limit (~2MB) publishes in
LargeMessagePublishIntegrationTests.kt uses short await windows (await(seconds =
10) and another at lines referenced 218-221) which are brittle; increase these
timeout values (e.g., to 30s or 60s) for the await(...) calls in the failing
publish assertions so the lambda that checks
result.isFailure/result.getOrThrow().timetoken has a larger budget in
integration environments—update each await(...) invocation in
LargeMessagePublishIntegrationTests.kt that wraps the publish assertions
accordingly.
- Around line 95-123: The test registers an anonymous SubscribeCallback via
pubnub.addListener and subscribes with pubnub.subscribeToBlocking but never
removes the listener or cleans up the ad-hoc client/observer on the success
path, risking leaked callbacks and shared state; update the test to capture the
listener instance (instead of an anonymous object), ensure after
success.listen() you call pubnub.removeListener(theListener) and unsubscribe
(e.g., pubnub.unsubscribeFromBlocking or pubnub.unsubscribe) from
expectedChannel, and also shut down/close the observer/ad-hoc client (call its
close/stop/shutdown method) so both listener and client resources are always
torn down on the test path (do the same for the other test path around lines
205-222).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8da568ce-94ac-46e4-a6d4-888bbe885316
📒 Files selected for processing (4)
pubnub-gson/pubnub-gson-impl/src/integrationTest/java/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.javapubnub-gson/pubnub-gson-impl/src/integrationTest/java/com/pubnub/api/integration/PublishIntegrationTests.javapubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.ktpubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/PublishIntegrationTests.kt
✅ Files skipped from review due to trivial changes (1)
- pubnub-gson/pubnub-gson-impl/src/integrationTest/java/com/pubnub/api/integration/PublishIntegrationTests.java
Comment on lines
+95
to
+123
| pubnub.addListener( | ||
| object : SubscribeCallback() { | ||
| override fun status( | ||
| pubnub: PubNub, | ||
| pnStatus: PNStatus, | ||
| ) { | ||
| } | ||
|
|
||
| override fun message( | ||
| pubnub: PubNub, | ||
| event: PNMessageResult, | ||
| ) { | ||
| if (event.message.asString == largeMessage) { | ||
| success.set(true) | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
|
|
||
| pubnub.subscribeToBlocking(expectedChannel) | ||
|
|
||
| observer.publish( | ||
| message = largeMessage, | ||
| channel = expectedChannel, | ||
| usePost = true, | ||
| ).sync() | ||
|
|
||
| success.listen() | ||
| } |
There was a problem hiding this comment.
Tear down listeners and ad-hoc clients after each test path.
The listener/client lifecycle isn’t closed in these paths, which can leak callbacks/resources and affect subsequent tests.
Suggested change
- pubnub.addListener(
- object : SubscribeCallback() {
+ val listener =
+ object : SubscribeCallback() {
override fun status(
pubnub: PubNub,
pnStatus: PNStatus,
) {
}
@@
- },
- )
+ }
+ pubnub.addListener(listener)
- pubnub.subscribeToBlocking(expectedChannel)
-
- observer.publish(
- message = largeMessage,
- channel = expectedChannel,
- usePost = true,
- ).sync()
-
- success.listen()
+ try {
+ pubnub.subscribeToBlocking(expectedChannel)
+ observer.publish(
+ message = largeMessage,
+ channel = expectedChannel,
+ usePost = true,
+ ).sync()
+ success.listen()
+ } finally {
+ pubnub.removeListener(listener)
+ pubnub.unsubscribeAll()
+ observer.destroy()
+ }- pubnubWithCrypto.publish(
- channel = expectedChannel,
- message = largeMessage,
- usePost = true,
- ).await(seconds = 15) { result ->
- assertFalse(result.isFailure)
- assertTrue(result.getOrThrow().timetoken > 0)
- }
+ try {
+ pubnubWithCrypto.publish(
+ channel = expectedChannel,
+ message = largeMessage,
+ usePost = true,
+ ).await(seconds = 15) { result ->
+ assertFalse(result.isFailure)
+ assertTrue(result.getOrThrow().timetoken > 0)
+ }
+ } finally {
+ pubnubWithCrypto.destroy()
+ }As per coding guidelines, pubnub-kotlin/pubnub-kotlin-impl/**: "Verify proper error handling and resource cleanup." and "Check for potential memory leaks in subscription/listener management.", and **/*Test*: "Ensure proper test isolation."
Also applies to: 205-222
🧰 Tools
🪛 detekt (1.23.8)
[warning] 100-101: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.kt`
around lines 95 - 123, The test registers an anonymous SubscribeCallback via
pubnub.addListener and subscribes with pubnub.subscribeToBlocking but never
removes the listener or cleans up the ad-hoc client/observer on the success
path, risking leaked callbacks and shared state; update the test to capture the
listener instance (instead of an anonymous object), ensure after
success.listen() you call pubnub.removeListener(theListener) and unsubscribe
(e.g., pubnub.unsubscribeFromBlocking or pubnub.unsubscribe) from
expectedChannel, and also shut down/close the observer/ad-hoc client (call its
close/stop/shutdown method) so both listener and client resources are always
torn down on the test path (do the same for the other test path around lines
205-222).
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.kt (2)
196-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncrease timeout budget for near-2MB publish tests.
Line 196 (
10s) and Line 218 (15s) are tight for integration environments with large payloads and can intermittently fail. Use a larger budget (e.g., 60s) to reduce flakiness.Suggested minimal fix
- ).await(seconds = 10) { result -> + ).await(seconds = 60) { result -> @@ - ).await(seconds = 15) { result -> + ).await(seconds = 60) { result ->As per coding guidelines,
**/*Test*: "Check for flaky test patterns (timing, ordering dependencies)."Also applies to: 218-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.kt` around lines 196 - 199, The near-2MB publish integration tests in LargeMessagePublishIntegrationTests use too-small await timeouts (the await(seconds = 10) { ... } block and another await(seconds = 15) { ... } block) which causes flakiness; increase these timeout budgets to a larger value (e.g., await(seconds = 60) { ... }) in both places where the publish result is asserted (the lambda containing assertFalse(result.isFailure) and assertTrue(result.getOrThrow().timetoken > 0)) so the integration environment has enough time to complete large payload publishes.
95-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure listener/client resources are always torn down.
Line 95 adds a listener and Line 93/Line 205 create ad-hoc clients, but there is no guaranteed cleanup path. This can leak callbacks/clients and pollute later tests. Wrap both flows in
try/finallyand alwaysremoveListener,unsubscribeAll(or channel unsubscribe), anddestroy().Suggested minimal fix
- pubnub.addListener( - object : SubscribeCallback() { + val listener = + object : SubscribeCallback() { override fun status( pubnub: PubNub, pnStatus: PNStatus, ) { } @@ - }, - ) - - pubnub.subscribeToBlocking(expectedChannel) - - observer.publish( - message = largeMessage, - channel = expectedChannel, - usePost = true, - ).sync() - - success.listen() + } + pubnub.addListener(listener) + + try { + pubnub.subscribeToBlocking(expectedChannel) + observer.publish( + message = largeMessage, + channel = expectedChannel, + usePost = true, + ).sync() + success.listen() + } finally { + pubnub.removeListener(listener) + pubnub.unsubscribeAll() + observer.destroy() + } @@ - pubnubWithCrypto.publish( - channel = expectedChannel, - message = largeMessage, - usePost = true, - ).await(seconds = 15) { result -> - assertFalse(result.isFailure) - assertTrue(result.getOrThrow().timetoken > 0) - } + try { + pubnubWithCrypto.publish( + channel = expectedChannel, + message = largeMessage, + usePost = true, + ).await(seconds = 15) { result -> + assertFalse(result.isFailure) + assertTrue(result.getOrThrow().timetoken > 0) + } + } finally { + pubnubWithCrypto.destroy() + }As per coding guidelines,
pubnub-kotlin/pubnub-kotlin-impl/**: "Verify proper error handling and resource cleanup." and "Check for potential memory leaks in subscription/listener management.", and**/*Test*: "Ensure proper test isolation."Also applies to: 205-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.kt` around lines 95 - 123, Wrap the subscription/publish flows in try/finally to guarantee teardown: after registering the anonymous SubscribeCallback via pubnub.addListener(...) and calling pubnub.subscribeToBlocking(...), ensure in a finally block you call pubnub.removeListener(the same SubscribeCallback), pubnub.unsubscribeAll() or pubnub.unsubscribe(expectedChannel), and pubnub.destroy() for any ad-hoc PubNub clients created (the ones around lines creating observer and other ad-hoc clients); similarly ensure any observer/client created for publish has its destroy() invoked in finally. Use the same SubscribeCallback reference (not an anonymous inline only) so removeListener can be called, and perform unsubscribe/destroy in finally to avoid leaking listeners/clients across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@pubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.kt`:
- Around line 196-199: The near-2MB publish integration tests in
LargeMessagePublishIntegrationTests use too-small await timeouts (the
await(seconds = 10) { ... } block and another await(seconds = 15) { ... } block)
which causes flakiness; increase these timeout budgets to a larger value (e.g.,
await(seconds = 60) { ... }) in both places where the publish result is asserted
(the lambda containing assertFalse(result.isFailure) and
assertTrue(result.getOrThrow().timetoken > 0)) so the integration environment
has enough time to complete large payload publishes.
- Around line 95-123: Wrap the subscription/publish flows in try/finally to
guarantee teardown: after registering the anonymous SubscribeCallback via
pubnub.addListener(...) and calling pubnub.subscribeToBlocking(...), ensure in a
finally block you call pubnub.removeListener(the same SubscribeCallback),
pubnub.unsubscribeAll() or pubnub.unsubscribe(expectedChannel), and
pubnub.destroy() for any ad-hoc PubNub clients created (the ones around lines
creating observer and other ad-hoc clients); similarly ensure any
observer/client created for publish has its destroy() invoked in finally. Use
the same SubscribeCallback reference (not an anonymous inline only) so
removeListener can be called, and perform unsubscribe/destroy in finally to
avoid leaking listeners/clients across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5dfdfabf-c6bc-4ab4-a784-94eef822332d
📒 Files selected for processing (4)
pubnub-gson/pubnub-gson-impl/src/integrationTest/java/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.javapubnub-kotlin/pubnub-kotlin-impl/src/integrationTest/kotlin/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.ktpubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/api/PubNubUtilTest.ktpubnub-kotlin/pubnub-kotlin-impl/src/test/kotlin/com/pubnub/internal/suite/pubsub/PublishV2PostMapPayloadTestSuite.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- pubnub-gson/pubnub-gson-impl/src/integrationTest/java/com/pubnub/api/integration/LargeMessagePublishIntegrationTests.java
…ation for messages send via Publish, Signal and received messages and signals in subscribe. Added config property loggedMessageContentMaxBytes that limits size of message content logged. Added fix limit 24KB for http response message being logged. Added logging for received message and signals.
…ation for publishFileMessage Added config property loggedMessageContentMaxBytes that limits size of message content logged.
…ontentMaxBytes and new property loggedHttpResponseMaxBytes having default 2000
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: Send messages up to 2 MB
feat: Reject oversized publishes client-side with a clear error
feat: New LogContentConfig with loggedMessageContentMaxBytes + loggedHttpResponseMaxBytes (0 disables, <0 unlimited)
feat: Fingerprint on publish, signal, subscribe and publishFileMessage debug logs