Implement new service type changes and client changes #13

Bhashinee · 2021-01-11T11:30:28Z

Purpose

This PR addresses following issues,

Align asynchronous client according to the new design
Query param, path param support for WebSocket upgrade service.
Custom headers support for WebSocket upgrade service.
Introducing new Sync client
Make the server init function return errors
Take the http:Listener as an input for service init
Rename the websocket:WebSocketError to websocket:Error
Change the pushText to writeString, pushBinary to writeBytes, onText to onString, onBinary to onBytes, onOpen to onConnect
Change the asynchronous client init to take the callbackservice as an input and remove the callbackService config from client configuration
Make the upgrade service to have one resource instead of a remote function

Samples

websocket service

listener websocket:Listener l2 = checkpanic new(21003);

service /onTextString on l2 {
   resource function get .() returns websocket:Service|websocket:UpgradeError {
       return new WsService1();
   }
}

service class WsService1 {
  *websocket:Service;
  remote isolated function onString(websocket:Caller caller, string data) {
      checkpanic caller->writeString(data);
  }
}

Websocket Async client

service class clientPushCallbackService {
    *websocket:Service;
    remote function onString(websocket:AsyncClient wsEp, string text) {
        data = <@untainted>text;
    }

    remote isolated function onError(websocket:AsyncClient wsEp, error err) {
        io:println(err);
    }
}

websocket:AsyncClient wsClient = new("ws://localhost:21003/onTextString/", new clientPushCallbackService());

Websocket Sync client

Read string

websocket:SyncClient wsClient = new("ws://localhost:21000/onTextString");
var resp1 = wsClient->writeString("Hi world1");
string resp2 = check wsClient->readString(string);

Read binary data
byte[] resp3 = check wsClient->readBytes();

…allerina-websocket into http-history

Removed the callbackService from client configuration and added it as a optional parameter in the client init.

…allerina-websocket into http-history

Bhashinee · 2021-01-13T02:56:24Z

websocket-ballerina/tests/websocket_on_binary_continuation.bal

@@ -1,75 +1,73 @@
-//// Copyright (c) 2020 WSO2 Inc. (//www.wso2.org) All Rights Reserved.


Need to think of a way to rewrite this test once this issue gets fixed. Kept the test commented for reference.

Bhashinee · 2021-01-13T02:56:40Z

websocket-ballerina/tests/websocket_server_exceptions.bal

@@ -1,61 +1,62 @@
-//// Copyright (c) 2020 WSO2 Inc. (http://www.wso2.org) All Rights Reserved.


Need to think of a way to rewrite this test once this issue gets fixed. Kept the test commented for reference.

shafreenAnfar · 2021-01-13T03:25:11Z

websocket-ballerina/Package.md

-}
-resource function upgrader(http:Caller caller, http:Request req, string name) {
+service /basePath on new websocket:Listener(21003) {
+    resource function onUpgrade .(http:Caller caller, http:Request req) returns websocket:Service|websocket:UpgradeError {


Is this correct ? I thought it should be

resource function get .(http:Caller caller, http:Request req) returns websocket:Service|websocket:UpgradeError

It is the resource(method) name, right? In HTTP it is mapped to the accessor get, post, etc. In WebSocket, we always get a specific get request which is always dispatched to this single resource. I think it is clear to have onUpgrade as the name in websocket context.

Actually what comes first is the accessor name. @chamil321 can you confirm this ?

Yes, The first identifier is the accessor name. The second one is the resource name(path). In this case she has used onUpgrade as accessor name which is a custom accessor. Here the HTTP dispatching logic does not apply as webscoket as its own implementation.

IMO, this could lead to confusions as we maintain all the HTTP semantics in this resource. Initial request is HTTP and HTTP does not have an onUpgrade verb.

@Bhashinee it is not the resource name. So what we need is

resource function get .(http:Caller caller, http:Request req) returns websocket:Service|websocket:UpgradeError

shafreenAnfar · 2021-01-13T03:27:53Z

websocket-ballerina/Package.md

-    }
-}
-resource function upgrader(http:Caller caller, http:Request req, string name) {
+service /basePath on new websocket:Listener(21003) {


If the base path is not given does it default to /ws ?

The default is '/'. ws://localhost:21029 or ws://localhost:21029/ will get dispatched to this service.

I think it should default to /ws as that is most common case ? wdyt ?

As per the discussion had, we decided not to do this for now.

shafreenAnfar · 2021-01-13T03:33:00Z

websocket-ballerina/Package.md


-**onOpen resource**: As soon as the WebSocket handshake is completed and the connection is established, the `onOpen` resource is dispatched. This resource is only available in the service of the server.
+**onConnect resource**: As soon as the WebSocket handshake is completed and the connection is established, the `onConnect` resource is dispatched. This resource is only available in the service of the server.


Is it resource or remote ?

It is a remote method. Will change this.

shafreenAnfar · 2021-01-13T03:34:53Z

websocket-ballerina/service_endpoint.bal

-    public isolated function init(int port, ListenerConfiguration? config = ()) {
+    # + port - Listening port of the websocket service listener
+    # + config - Configurations for the websocket service listener
+    public isolated function init(int|http:Listener listenerPort, ListenerConfiguration? config = ()) returns Error? {


Let's say listener not listenerport

shafreenAnfar · 2021-01-13T03:37:09Z

websocket-ballerina/tests/03_push_and_onText.bal

-service UpgradeService /onTextString on new Listener(21003) {
-   remote isolated function onUpgrade(http:Caller caller, http:Request req) returns Service|WebSocketError {
+service /onTextString on l2 {
+   resource function onUpgrade .() returns Service|UpgradeError {


Is this correct ? Do we have an accessor named onUpgrade ?

Changed to get

shafreenAnfar · 2021-01-13T03:40:29Z

websocket-ballerina/tests/03_push_and_onText.bal

-  remote isolated function onText(Caller caller, string data, boolean finalFrame) {
-      checkpanic caller->pushText(data);
+  remote isolated function onString(Caller caller, string data) {
+      checkpanic caller->writeString(data);


Instead of panic I think it is better if we can check and return the error

shafreenAnfar · 2021-01-13T03:41:57Z

websocket-ballerina/tests/03_push_and_onText.bal

-service object {} clientPushCallbackService = service object {
-    remote function onText(Client wsEp, string text) {
+service class clientPushCallbackService {
+    remote function onString(AsyncClient wsEp, string text) {


Here what we need is the caller right not the AsyncClient.

The caller is for the server-side. Here we pass in the created WebSocket client.

Interesting. I thought we should expose the same client in the form of the caller.

I think it should be the caller even when it is on the client side. Is there a specific reason to do otherwise ?

We can rename this to Caller. But isn't it confusing for the users? They have created an AsyncClient and for sending messages they are using something called Caller?

As per the discussion had, renaming this to Caller

shafreenAnfar · 2021-01-13T04:08:19Z

websocket-ballerina/tests/29_ssl_echo.bal

-       }
-   }) {
-   remote isolated function onUpgrade(http:Caller caller, http:Request req) returns Service {
+listener Listener l7 = checkpanic new(21029, {


We don't need checkpanic here ? I see, it seems like the spec has changed. ballerina-platform/ballerina-spec#671

If we need it can't we use check instead of checkpanic ?

shafreenAnfar · 2021-01-13T04:08:42Z

websocket-ballerina/tests/29_ssl_echo.bal

+                     });
+
+service /sslEcho on l7 {
+   resource function upgrade .(http:Request req) returns Service {


…ation Migrate ballerina/http module

Bhashinee added 15 commits December 23, 2020 20:23

Implement to return multiple websocket services from upgrade service

cc75ffd

Merge branch 'main' of https://github.com/ballerina-platform/module-b…

57b79c5

…allerina-websocket into http-history

Fix checkstyle errors

7c619ff

Refactor websocket:Client to websocket:AsyncClient

7e3e9c7

Remove the callbackService configuration

4487651

Removed the callbackService from client configuration and added it as a optional parameter in the client init.

Rename pushText to writeString

85e3da9

Change pushBinary to writeBytes

d7f11a6

Rename onText and onBinary to onString and onBytes

5c4fdf9

Accept port or the http listener in websocket listener init

018254f

Change the remote function in upgrade service to a resource

b2af5e9

Sync client initial implementation

5365f1b

Refactor code and add readBytes action for Sync client

cf75a0a

Edit package.md

6921664

Merge branch 'main' of https://github.com/ballerina-platform/module-b…

f632d80

…allerina-websocket into http-history

Add synchronous client actions

ef0586e

Bhashinee force-pushed the http-history branch from 79212b0 to 39030ee Compare January 11, 2021 14:20

Refactor code

f605a8a

Bhashinee force-pushed the http-history branch from 39030ee to f605a8a Compare January 11, 2021 14:35

Rename onOpen to onConnect and make listener creation return errors

f92d754

Bhashinee force-pushed the http-history branch from e8e014b to f92d754 Compare January 12, 2021 01:38

Bhashinee added 6 commits January 12, 2021 07:30

Rename WebsocketError to Error

b628c73

Rename some constants

ae1180f

Remove data binding support for sync client

14e6444

Remove data binding support from Async client

1744a87

Remove data binding support in both read and write

3c5ba05

Wait until the final frame is received to return in binary

edeae8e

Bhashinee force-pushed the http-history branch from 7af0d81 to edeae8e Compare January 13, 2021 01:57

Bhashinee commented Jan 13, 2021

View reviewed changes

Make the aggregate string creation to use a string builder

5fd63ed

Bhashinee force-pushed the http-history branch from 3b3d6cb to 5fd63ed Compare January 13, 2021 03:08

shafreenAnfar reviewed Jan 13, 2021

View reviewed changes

Bhashinee added 4 commits January 13, 2021 15:40

Address review suggestions

1bd7c05

Change the use of runtime stdlib

0820177

Address review suggestions

a5fd002

Remove runtime Api dependencies

2b5eec6

Bhashinee force-pushed the http-history branch from 5afb3cd to 2b5eec6 Compare January 14, 2021 07:37

Add more test cases for corrupted frames and remove unnecessary codes

4482487

Bhashinee force-pushed the http-history branch from 65f8602 to 4482487 Compare January 15, 2021 06:13

Make the client init return errors

9989a2d

Bhashinee force-pushed the http-history branch from d17eadd to 9989a2d Compare January 15, 2021 07:01

shafreenAnfar approved these changes Jan 15, 2021

View reviewed changes

Bhashinee merged commit b9013dd into main Jan 15, 2021

Bhashinee pushed a commit to Bhashinee/module-ballerina-websocket that referenced this pull request May 3, 2021

Merge pull request ballerina-platform#13 from ballerina-platform/migr…

b7c2a60

…ation Migrate ballerina/http module

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new service type changes and client changes #13

Implement new service type changes and client changes #13

Bhashinee commented Jan 11, 2021 •

edited

Loading

Bhashinee Jan 13, 2021

Bhashinee Jan 13, 2021

shafreenAnfar Jan 13, 2021

Bhashinee Jan 13, 2021

shafreenAnfar Jan 13, 2021

chamil321 Jan 13, 2021

shafreenAnfar Jan 13, 2021 •

edited

Loading

shafreenAnfar Jan 13, 2021

Bhashinee Jan 13, 2021 •

edited

Loading

shafreenAnfar Jan 13, 2021

Bhashinee Jan 15, 2021

shafreenAnfar Jan 13, 2021

Bhashinee Jan 13, 2021

shafreenAnfar Jan 13, 2021 •

edited

Loading

shafreenAnfar Jan 13, 2021 •

edited

Loading

Bhashinee Jan 15, 2021

shafreenAnfar Jan 13, 2021

shafreenAnfar Jan 13, 2021

Bhashinee Jan 13, 2021

shafreenAnfar Jan 13, 2021

shafreenAnfar Jan 13, 2021

Bhashinee Jan 13, 2021

Bhashinee Jan 15, 2021

shafreenAnfar Jan 13, 2021

shafreenAnfar Jan 13, 2021

		@@ -1,75 +1,73 @@
		//// Copyright (c) 2020 WSO2 Inc. (//www.wso2.org) All Rights Reserved.

		@@ -1,61 +1,62 @@
		//// Copyright (c) 2020 WSO2 Inc. (http://www.wso2.org) All Rights Reserved.


		onOpen resource: As soon as the WebSocket handshake is completed and the connection is established, the `onOpen` resource is dispatched. This resource is only available in the service of the server.
		onConnect resource: As soon as the WebSocket handshake is completed and the connection is established, the `onConnect` resource is dispatched. This resource is only available in the service of the server.

Implement new service type changes and client changes #13

Implement new service type changes and client changes #13

Conversation

Bhashinee commented Jan 11, 2021 • edited Loading

Purpose

Samples

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafreenAnfar Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhashinee Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafreenAnfar Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

shafreenAnfar Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhashinee commented Jan 11, 2021 •

edited

Loading

shafreenAnfar Jan 13, 2021 •

edited

Loading

Bhashinee Jan 13, 2021 •

edited

Loading

shafreenAnfar Jan 13, 2021 •

edited

Loading

shafreenAnfar Jan 13, 2021 •

edited

Loading