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

Jump to bottom

Add Close Frame Record return type support #1497

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Add Close Frame Record return type support #1497

wants to merge 23 commits into from
+503 −14

Conversation

chathushkaayash
Copy link

@chathushkaayash chathushkaayash commented Feb 11, 2025 •
edited
Loading

Purpose

$subject

Resolves Add support for websocket close frame

Examples

1. Close the connection using NormalClosure

remote function onMessage(websocket:Caller caller, string data) returns websocket:NormalClosure {
    // ... omitted for brevity
    return websocket:NORMAL_CLOSURE;
}

2. Close the connection using CustomCloseFrame

remote function onMessage(websocket:Caller caller, string data) returns websocket:CustomCloseFrame {
    // ... omitted for brevity
    return {status: 3555, reason: "Custom close frame message"};
}

Checklist

  • Linked to an issue
  • Updated the specification
  • Updated the changelog
  • Added tests
  • Checked native-image compatibility

chathushkaayash and others added 12 commits February 11, 2025 14:50
@chathushkaayash
Add initial implementation
4a4316a
@chathushkaayash
Merge pull request #1 from ballerina-platform/main
96a2078
@chathushkaayash
[Automated] Update the native jar versions
42a174b
@chathushkaayash
Update licenses and remove parenthesis in if statements
cd92e80
@chathushkaayash @ballerina-bot
Sync the fork (#2)
02ff641 
* Update chat_application test results on Tue Feb 11 18:29:43 UTC 2025

* Update chat_application test results on Wed Feb 12 18:30:42 UTC 2025

---------

Co-authored-by: ballerina-bot <ballerina-bot@ballerina.org>
Co-authored-by: ballerina-bot <ballerinalang@wso2.com>
@chathushkaayash
Merge pull request #3 from ballerina-platform/main
df2600b 
Sync the fork
@chathushkaayash
Update close frame records based on new suggestions
0027a06
@chathushkaayash
Fix type casting for close frame reason in WebSocketResourceDispatcher
34f51f6
@chathushkaayash
Add ProtocolError close frame record and related tests
40eb6ea
@chathushkaayash
[Automated] Update the native jar versions
dc7e975
@chathushkaayash
Add close frame support tests for custom dispatcher methods
1db08cb
@chathushkaayash
Merge pull request #4 from ballerina-platform/main
401866b 
Sync the fork
@ayeshLK ayeshLK marked this pull request as ready for review February 19, 2025 04:52
@ayeshLK ayeshLK requested review from Bhashinee and removed request for shafreenAnfar and ThisaruGuruge February 19, 2025 04:52
ayeshLK
ayeshLK reviewed Feb 20, 2025
View reviewed changes
ayeshLK
ayeshLK reviewed Feb 20, 2025
View reviewed changes
ayeshLK
ayeshLK reviewed Feb 20, 2025
View reviewed changes
ayeshLK
ayeshLK previously approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@ayeshLK ayeshLK left a comment

Choose a reason for hiding this comment

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

LGTM

MohamedSabthar
MohamedSabthar previously approved these changes Mar 3, 2025
View reviewed changes
@chathushkaayash chathushkaayash dismissed stale reviews from MohamedSabthar and ayeshLK via 20eb8c2 March 3, 2025 04:43
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Mar 3, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Bhashinee
Bhashinee reviewed Mar 3, 2025
View reviewed changes
native/src/main/java/io/ballerina/stdlib/websocket/WebSocketResourceDispatcher.java
Comment on lines +1137 to +1139
result = wsService.getRuntime().callMethod(
connectionInfo.getWebSocketEndpoint(), WebSocketConstants.RESOURCE_NAME_CLOSE,
strandMetadata, status, reason);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we invoke the WebSocket close resource here?

Copy link
Author

Choose a reason for hiding this comment

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

When the user sends a close frame, we call this method to actually close the connection.

Copy link
Member

@Bhashinee Bhashinee Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but in this case we are the ones who send a close frame right? Not receiving a close frame. Isn't that correct?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but in this case we are the ones who send a close frame right? Not receiving a close frame. Isn't that correct?

Correct. We send the close frame.

Copy link
Author

Choose a reason for hiding this comment

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

Had an offline discussion with @Bhashinee and decided to implement a new sendCloseFrame method instead of reusing externClose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bhashinee Bhashinee Bhashinee left review comments

@MohamedSabthar MohamedSabthar MohamedSabthar left review comments

@ayeshLK ayeshLK ayeshLK left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for websocket close frame
4 participants
@chathushkaayash @Bhashinee @MohamedSabthar @ayeshLK