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.

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

added the wsf protocol #240

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

added the wsf protocol #240

wants to merge 3 commits into from

Conversation

mabels
Copy link

@mabels mabels commented Jun 16, 2016

Hi,

i added the wsf protocol to support the websocket to run over
socket files.
I still think it is alittle fishy but how you think we should define a
socketPath in a ws: url.

wsf:///absolute/path/to/socket/
wsf://./relative/path/to/socket/

i did not implemented the pathname of the url function. The pathname
on wsf protocols is this is set to the "kaputt".

I build a test which tests wsf and ws connections

Thx

Meno

@theturtle32
Copy link
Owner

This is new to me... I've never heard of a "wsf" protocol. Google search doesn't turn anything up either.

I'm not necessarily fundamentally opposed to extending the library to support using unix socket files, but If we do so, I want to make sure to do it carefully and in the best way possible. I'm not really much of a Windows guy, but Windows doesn't support socket files of any sort, does it? Is there an equivalent? If not, should we instrument some kind of error messaging/handling when someone tries to use that functionality on a platform that doesn't support it?

What's the use case for this?

@ibc - Do you have any thoughts or opinions about this?

@ibc
Copy link
Collaborator

ibc commented Jun 16, 2016

I have no idea regarding what "wsf" is. I understand that it intends to be a message boundary protocol (in fact, the WebSocket protocol) over Unix Sockets in STREAM mode.

Honestly, I don't understand why this stuff should be added into a WebSocket library.

@mabels
Copy link
Author

mabels commented Jun 16, 2016

I created the wsf protocol name by my own.
so it's just to make clear that the host part ist not a DNS name. We could
name it like we want. there is something out with http that looks like
http:://Unix://socket:/ but this is pretty ugly also.

the usecase could be to enable the communication between two node processes
running on the same machine. And wants to avoid the exposure of a IP:port.

I use it for a command line tool which talks to a background process like
SSH and ssh-agent

if you.look in my added code you will see that this is only a changes the
connection establishment. it uses features which are build in the http
client and server of node for ages now.

cheers

meno

On Thu, Jun 16, 2016, 22:37 Iñaki Baz Castillo [email protected]
wrote:

I have no idea regarding what "wsf" is. I understand that it intends to be
a message boundary protocol (in fact, the WebSocket protocol) over Unix
Sockets in STREAM mode.

Honestly, I don't understand why this stuff should be added into a
WebSocket library.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABLJuOsNdMb2__rZKqHT5xCZnsSdjVpks5qMbPmgaJpZM4I3T8z
.

@mabels
Copy link
Author

mabels commented Jun 16, 2016

for the windows guys it should also work, they have also named sockets. I
could try to test it tomorrow.

meno

On Thu, Jun 16, 2016, 23:21 Meno Abels [email protected] wrote:

I created the wsf protocol name by my own.
so it's just to make clear that the host part ist not a DNS name. We could
name it like we want. there is something out with http that looks like
http:://Unix://socket:/ but this is pretty ugly also.

the usecase could be to enable the communication between two node
processes running on the same machine. And wants to avoid the exposure of a
IP:port.

I use it for a command line tool which talks to a background process like
SSH and ssh-agent

if you.look in my added code you will see that this is only a changes the
connection establishment. it uses features which are build in the http
client and server of node for ages now.

cheers

meno

On Thu, Jun 16, 2016, 22:37 Iñaki Baz Castillo [email protected]
wrote:

I have no idea regarding what "wsf" is. I understand that it intends to
be a message boundary protocol (in fact, the WebSocket protocol) over Unix
Sockets in STREAM mode.

Honestly, I don't understand why this stuff should be added into a
WebSocket library.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABLJuOsNdMb2__rZKqHT5xCZnsSdjVpks5qMbPmgaJpZM4I3T8z
.

@@ -112,6 +112,40 @@ function WebSocketClient(config) {

util.inherits(WebSocketClient, EventEmitter);

function handleWsfProtocol(url) {
var path = require('path');
var fs = require('fs');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like at all this. Modules should not be loaded within functions but on the top of the file.

Copy link
Author

Choose a reason for hiding this comment

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

no problem, let's move it out

meno

On Thu, Jun 16, 2016, 23:26 Iñaki Baz Castillo [email protected]
wrote:

In lib/WebSocketClient.js
#240 (comment)
:

@@ -112,6 +112,40 @@ function WebSocketClient(config) {

util.inherits(WebSocketClient, EventEmitter);

+function handleWsfProtocol(url) {

  • var path = require('path');
  • var fs = require('fs');

I don't like at all this. Modules should not be loaded within functions
but on the top of the file.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/theturtle32/WebSocket-Node/pull/240/files/91c9b1d2ab8bb696192a7ee7b63bcafb982ef665#r67427092,
or mute the thread
https://github.com/notifications/unsubscribe/AABLJvfQ3PNafTCAYIORlUE8TvYZUxzyks5qMb-GgaJpZM4I3T8z
.

@ibc
Copy link
Collaborator

ibc commented Jun 16, 2016

Well. I must say that this feature may be interesting. It still is "WebSocket", it's just that the transport is not TCP but named sockets.

Node.js provides inter-process communication mechanisms, but that is just for processes spawned by a parent process.

Node.js uses libuv internally, which has a uv_stream_t handler that manages both TCP connection or Unix Sockets in STREAM mode in the very same way, so the feature in this PR looks like the same abstraction at a higher level.

Adding @saghul to the conversation as he probably has something to add.

@mabels
Copy link
Author

mabels commented Jun 16, 2016

just some thoughts about wsf what is if we switch back to wss and ws and
adapt the ipv6 literal nr notation for our needs like:

ws://[/the/file]

if will change this tomorrow, with the windows test

meno

On Thu, Jun 16, 2016, 23:42 Iñaki Baz Castillo [email protected]
wrote:

Well. I must say that this feature may be interesting. It still is
"WebSocket", it's just that the transport is not TCP but named sockets.

Node.js provides inter-process communication mechanisms, but that is just
for processes spawned by a parent process.

Node.js uses libuv internally, which has a uv_stream_t handler that
manages both TCP connection or Unix Sockets in STREAM mode in the very
same way, so the feature in this PR looks like the same abstraction at
a higher level.

Adding @saghul https://github.com/saghul to the conversation as he
probably has something to add.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABLJtyBzxbLcvhV95QeLZxptZdvE9C2ks5qMcNGgaJpZM4I3T8z
.

@saghul
Copy link

saghul commented Jun 16, 2016

Hi there! I haven't looked at the code, but what you propose has been used in the Python world for a while, so there is precedent. Python uses WSGI (sort of CGI) for webservers and indeed it makes some sense to communicate over a Unix domain socket instead of using localhost.

As for the URI identifier I'd personally go for something more explicit like ws-unix:// but I don't have a horse in this race :-)

Some bonus nachos for Linux: you can use abstract sockets and also avoid using a file for the Unix socket. See https://github.com/saghul/node-abstractsocket, written by yours truly :-)

Hope that helps!

mabels added 2 commits June 17, 2016 12:34
* if the hostname is in the ipv6 literal form [::1] i allow
  to write in the inner part the socket name. So the url could
  look like: ws://[/path/to/socket]/rest
* removed the hostname evaluation. There is a host attribute
  in a parsed url instance.
@mabels
Copy link
Author

mabels commented Jun 17, 2016

So I tested on windows and it works only the pipe names are special:

?\pipe\path\to\pipe.file

but the rest is fine.

Do you think you will merge this in the main line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants