Skip to content
This repository was archived by the owner on Nov 22, 2024. It is now read-only.

feat(socket-engine): introduce package #999

Merged
merged 2 commits into from
Aug 15, 2018
Merged

Conversation

Toxicable
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Feature

@CaerusKaru CaerusKaru added state: WIP need: discussion On the agenda for team meeting to determine next steps target: minor target: minor This PR is targeted for the next minor release labels May 20, 2018
@Toxicable Toxicable force-pushed the fabian/socket-engine branch 2 times, most recently from 148b9ef to 313dd54 Compare May 25, 2018 20:10
@Toxicable Toxicable force-pushed the fabian/socket-engine branch from 313dd54 to 7b7b44a Compare June 8, 2018 21:14
@Toxicable Toxicable added action: review and removed state: WIP state: blocked need: discussion On the agenda for team meeting to determine next steps target: minor target: minor This PR is targeted for the next minor release labels Jun 9, 2018
@Toxicable Toxicable force-pushed the fabian/socket-engine branch from 269b07d to 775e38a Compare June 9, 2018 23:10
@CaerusKaru CaerusKaru added action: cleanup target: minor target: minor This PR is targeted for the next minor release comp: socket-engine state: WIP and removed action: review labels Jun 10, 2018
@Toxicable Toxicable force-pushed the fabian/socket-engine branch from 775e38a to 10c1d21 Compare June 22, 2018 20:16
@Toxicable Toxicable changed the title WIP: feat(socket-engine): introduce package feat(socket-engine): introduce package Jul 27, 2018
## Usage Client

Your client can be whatever language, framework or platform you like.
Aslong as it can connect to a TCP Socket (which all frameworks can) then you're good to go.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: As long

import * as net from 'net';

const client = net.createConnection(9090, 'localhost', () => {
const renderOptions = {id: 1, url: '/path',
Copy link
Contributor

Choose a reason for hiding this comment

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

This example makes it appear as if there is only one message sent per createConnection with id of 1.

Maybe separate out the connection from sending the request?

const server = net.createServer(socket => {
socket.on('data', async buff => {
const message = buff.toString();
const renderOptions = JSON.parse(message) as SocketEngineRenderOptions;
Copy link

Choose a reason for hiding this comment

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

Hi all

I've been playing around with the socket-engine a bit and noticed an in my opinion significant error. My setup: Connect to the socket from an Apache Tomcat container. I am honestly new to sockets and followed this guide, ended up something like this.

WebSocketClient simpleWebSocketClient = new StandardWebSocketClient();
List<Transport> transports = new ArrayList<>(1);
transports.add(new WebSocketTransport(simpleWebSocketClient));
SockJsClient sockJsClient = new SockJsClient(transports);

WebSocketStompClient stompClient = new WebSocketStompClient(sockJsClient);
stompClient.setMessageConverter(new MappingJackson2MessageConverter());

String url = "ws://localhost:9090/";
StompSessionHandler sessionHandler = new MyStompSessionHandler();
StompSession session = stompClient.connect(url, sessionHandler).get();

Now the problem is in the implementation of the Apache org.apache.tomcat.websocket.WsWebSocketContainer.java which sends some default parameters on the initial request. You can see the connect statement and it's unskippable parameters in this file on lines 266 to 275.

So here we are now, sending an initial request to the websocket. An informational request which is not in JSON format. This line tries to parse it - and dos not return an error -> The connection attempt ends in a timeout and the webapplication failes to start!

As far as I see you should cover this line by a try-catch too and handle errors in a maner way?

I'd like to state out once more that this is just what I noticed in a brief test. Also I'm not an Apache pro, just looked into the source code and didn't see a way to skip the initial request (except writing my own implementation, which is maybe a bit too overkill). Just wanted to let you know what I detected. It may be intentional that this line isn't covered by a try-catch, it may not. Up to you.

Cheers

Copy link
Author

Choose a reason for hiding this comment

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

That inital request is actually setting up the WS conenction.

// Create the initial HTTP request to open the WebSocket connection

A WebSocket connection starts out as a normal Http connection and once it figures out that both the client and server support WS it's Upgraded to a WS connection, this is what that inital connection is doing.

HOWEVER; We are not using WebSockets here.

We are using raw TCP Sockets, meaning, no http, no WS, only a raw TCP Socket.
I don't know what it's called in Java but it isn't WebSockets

Copy link

Choose a reason for hiding this comment

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

Oh that's the thing. Sorry and thank you for the explanation.

@Toxicable Toxicable force-pushed the fabian/socket-engine branch from ea6f7b1 to 2946ee3 Compare August 10, 2018 20:51
@@ -0,0 +1,13 @@
App - Your Angular application
Copy link
Author

Choose a reason for hiding this comment

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

remove this

url = "https://github.com/bazelbuild/rules_nodejs/archive/0.9.1.zip",
strip_prefix = "rules_nodejs-0.9.1",
sha256 = "6139762b62b37c1fd171d7f22aa39566cb7dc2916f0f801d505a9aaf118c117f",
# TODO: upgrade once https://github.com/bazelbuild/rules_nodejs/issues/218#issuecomment-395826361 is released
Copy link
Author

Choose a reason for hiding this comment

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

resolve this

Copy link
Author

Choose a reason for hiding this comment

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

ran into some other issues with this change, will nbeed a full set of updates later

Copy link
Member

@CaerusKaru CaerusKaru left a comment

Choose a reason for hiding this comment

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

LGTM

@CaerusKaru CaerusKaru merged commit de33b02 into master Aug 15, 2018
@CaerusKaru CaerusKaru deleted the fabian/socket-engine branch August 15, 2018 21:07
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup action: review target: minor target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants