chore: add @Blocking and @NonBlocking annotations#1192
Conversation
WalkthroughThe changes add JetBrains Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/main/java/io/ably/lib/rest/Auth.java (1)
1019-1024:⚠️ Potential issue | 🟠 Major
@NonBlockingonrenew()is incorrect — the method blocks on HTTP I/O.
renew()synchronously callsassertValidToken(...)(line 1021), which internally callsrequestToken(...)— a method you've annotated@Blockingbecause it performs HTTP requests. This makesrenew()a blocking method.The same issue applies to
renewAuth()on line 1034–1038: it also synchronously callsassertValidToken(...)before dispatching the async part.Both should be
@Blocking.Proposed fix
`@Deprecated` - `@NonBlocking` + `@Blocking` public TokenDetails renew() throws AblyException {- `@NonBlocking` + `@Blocking` public void renewAuth(RenewAuthResult result) throws AblyException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/main/java/io/ably/lib/rest/Auth.java` around lines 1019 - 1024, The `@NonBlocking` annotation on renew() (and likewise on renewAuth()) is incorrect because renew() calls assertValidToken(...), which invokes requestToken(...) and performs HTTP I/O; change the annotation to `@Blocking` for renew() and renewAuth() so their declared blocking behavior matches the synchronous HTTP calls — update the annotations on the methods renew() and renewAuth() (and any related javadoc/comments) to `@Blocking` and ensure no `@NonBlocking` remains on those methods that call assertValidToken(...) / requestToken(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/src/main/java/io/ably/lib/rest/Auth.java`:
- Around line 1019-1024: The `@NonBlocking` annotation on renew() (and likewise on
renewAuth()) is incorrect because renew() calls assertValidToken(...), which
invokes requestToken(...) and performs HTTP I/O; change the annotation to
`@Blocking` for renew() and renewAuth() so their declared blocking behavior
matches the synchronous HTTP calls — update the annotations on the methods
renew() and renewAuth() (and any related javadoc/comments) to `@Blocking` and
ensure no `@NonBlocking` remains on those methods that call assertValidToken(...)
/ requestToken(...).
There was a problem hiding this comment.
Pull request overview
This pull request adds @Blocking and @NonBlocking annotations from org.jetbrains.annotations to various public methods in the Ably Java SDK to improve documentation and clarity about thread-blocking behavior. The annotations help developers understand whether methods will block the calling thread or execute asynchronously with callbacks.
Changes:
- Added blocking behavior annotations to REST channel operations (history retrieval, presence operations, message publishing)
- Added blocking behavior annotations to authentication methods (authorize, requestToken, createTokenRequest, renew, renewAuth)
- Added non-blocking behavior annotations to realtime channel operations (attach, detach, subscribe, unsubscribe, setOptions)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/src/main/java/io/ably/lib/rest/ChannelBase.java | Added @Blocking and @NonBlocking annotations to REST channel methods including publish, history, and presence operations |
| lib/src/main/java/io/ably/lib/rest/Auth.java | Added @Blocking annotations to synchronous auth methods and @NonBlocking to async renewal methods (with issues identified) |
| lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | Added @NonBlocking annotations to realtime channel operations and @Blocking to synchronous history methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @NonBlocking | ||
| public TokenDetails renew() throws AblyException { | ||
| TokenDetails tokenDetails = assertValidToken(this.tokenParams, this.authOptions, true); | ||
| ably.onAuthUpdated(tokenDetails.token, false); | ||
| return tokenDetails; | ||
| } |
There was a problem hiding this comment.
The @NonBlocking annotation is incorrect for this method. The renew() method calls assertValidToken() which internally calls requestToken(), a blocking method that makes synchronous HTTP requests. This means renew() will actually block the calling thread despite being annotated as non-blocking. Consider changing the annotation to @Blocking to accurately reflect the method's behavior.
There was a problem hiding this comment.
@coderabbitai can you check if this is correct? Also, check other public methods that should be marked Blocking / NonBlocking ?
There was a problem hiding this comment.
@sacOO7, Copilot is correct. The renew() method is incorrectly annotated as @NonBlocking. It calls assertValidToken() which internally calls requestToken() (a blocking HTTP operation), so it should be @Blocking.
Here's a comprehensive review of public methods in the Auth class that need annotation corrections:
Methods with incorrect annotations:
renew()- Currently@NonBlocking, should be@Blocking(callsassertValidToken()→requestToken())renewAuth(RenewAuthResult)- Currently@NonBlocking, should be@Blocking(also callsassertValidToken()→requestToken())
Public methods missing annotations that should be added:
Should be @Blocking:
getAuthParams()- callsassertValidToken()in the token auth caseassertAuthorizationHeader(boolean)- may callrenew()orassertValidToken()
Should be @NonBlocking:
getAuthMethod()- simple gettergetBasicCredentials()- simple gettergetAuthOptions()- simple getter returning a copygetTokenDetails()- simple gettergetEncodedToken()- simple gettergetAuthorizationHeader()- simple getteronAuthError(ErrorInfo)- simple void method with local operationssetClientId(String)- setter with validationcheckClientId(BaseMessage, boolean, boolean)- validation logic onlyserverTimestamp()- simple calculationclearCachedServerTime()- simple field reset
The currently annotated methods (authorize, authorise, requestToken, createTokenRequest) are correctly marked as @Blocking.
| @NonBlocking | ||
| public void renewAuth(RenewAuthResult result) throws AblyException { | ||
| final TokenDetails tokenDetails = assertValidToken(this.tokenParams, this.authOptions, true); | ||
|
|
||
| ably.onAuthUpdatedAsync(tokenDetails.token, (success, errorInfo) -> result.onCompletion(success,tokenDetails,errorInfo)); | ||
| } |
There was a problem hiding this comment.
The @NonBlocking annotation is incorrect for this method. The renewAuth() method calls assertValidToken() which internally calls requestToken(), a blocking method that makes synchronous HTTP requests. This means renewAuth() will block the calling thread before invoking the callback asynchronously. Consider changing the annotation to @Blocking to accurately reflect the method's behavior, or refactor the method to avoid blocking operations on the calling thread.
There was a problem hiding this comment.
@coderabbitai can you check if this is correct? Also, check other public methods that should be marked Blocking / NonBlocking ?
There was a problem hiding this comment.
@sacOO7, I've analyzed the annotations. Copilot is correct - the @NonBlocking annotation on renewAuth() is incorrect.
Issue with renewAuth()
The renewAuth() method calls assertValidToken() synchronously, which internally calls requestToken() (a blocking HTTP operation). This means the calling thread blocks until the token is obtained, even though the final callback is invoked asynchronously. It should be marked @Blocking.
Other methods needing annotation fixes:
1. renew() method (line ~1019) - Currently marked @NonBlocking but should be @Blocking:
`@Deprecated`
`@NonBlocking` // ❌ Incorrect
public TokenDetails renew() throws AblyException {
TokenDetails tokenDetails = assertValidToken(...); // This blocks!
ably.onAuthUpdated(tokenDetails.token, false);
return tokenDetails;
}2. getAuthParams() method - Missing annotation, should be @Blocking:
- In the token auth case, it calls
assertValidToken()which can make blocking HTTP requests
3. assertAuthorizationHeader(boolean forceRenew) method - Missing annotation, should be @Blocking:
- Calls
renew()andassertValidToken(), both of which can block
Summary of needed changes:
renewAuth(): Change@NonBlocking→@Blockingrenew(): Change@NonBlocking→@BlockinggetAuthParams(): Add@BlockingassertAuthorizationHeader(): Add@Blocking
Simple getter methods like getAuthMethod(), getAuthorizationHeader(), etc. don't need annotations since they just return fields or perform quick calculations without I/O.
2 participants
Added
@Blockingand@NonBlockingannotations for better documentationSummary by CodeRabbit