WIP: Replace websocket-stream with ws #413
Conversation
|
@mcollina I'm getting an error on CI that seems caused by protocol decoder:
I see that in And protocol decoder fails here: var socket = conn.socket || conn
....
proto.port = socket._socket.remotePort
ipFamily = socket._socket.remoteFamilyIn poor words how can I get remotePort and remoteFamily props using the ws stream? I think this should be fixed in protocol decoder so |
|
it should be there somewhere, dig deep |
|
It should be there but can't find it 😕 https://nodejs.org/api/net.html#net_socket_remoteaddress |
|
@mcollina This way it works but I much prefer websocket-stream way |
Pull Request Test Coverage Report for Build f991d0e6ae895de4310f48e734d75f5b17bb1404-PR-413
💛 - Coveralls |
|
The implementation of Note: we should ask about adding the socket property to |
|
What about create an helper method un aedes to support it? Something like the method used in test/helper?
Daniel
… On 6 Feb 2020, at 19:20, Matteo Collina ***@***.***> wrote:
The implementation of ws is significantly better than the one in websocket-stream. We might want to update the readme with this change as well.
Note: we should ask about adding the socket property to ws streams.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@mcollina If you check the issue on ws module I asked to add the property but they told me that they won't do it (check link for more info). Should I also update examples? |
|
I think we should publish an utility module for this purpose. IMHO this is very useful.
Il 7 feb 2020, 16:50 +0100, Daniel Lando <notifications@github.com>, ha scritto:
… @mcollina If you check the issue on ws module I asked to add the property but they told me that they won't do it (check link for more info). Should I also update examples?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
You mean “aedes-websockets” plugin?
It should work like:
var websocket = require(“aedes-websockets”)
websocket(server, broker)
?
Daniel
… On 7 Feb 2020, at 19:25, Matteo Collina ***@***.***> wrote:
I think we should publish an utility module for this purpose. IMHO this is very useful.
Il 7 feb 2020, 16:50 +0100, Daniel Lando ***@***.***>, ha scritto:
> @mcollina If you check the issue on ws module I asked to add the property but they told me that they won't do it (check link for more info). Should I also update examples?
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@mcollina So what with this? |
|
I would just create a tiny module that offers the same API of websocket-stream server: https://www.npmjs.com/package/websocket-stream#on-the-server |
|
How about we create a factory createServer method to provide websocket, tls function? |
|
I’m +1 for this and also for the module option, the downside of this is that it would require to add websocket as dependency and some users would not need it.
Daniel
… On 10 Feb 2020, at 18:32, Gnought ***@***.***> wrote:
How about we create a factory createServer method to provide websocket, tls function?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
We could build a tiny module to include it |
|
I can do it tomorrow...
Daniel
… On 10 Feb 2020, at 20:13, Gnought ***@***.***> wrote:
We could build a tiny module to include it
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Maybe we could follow what fastify did. https://github.com/fastify/fastify/blob/master/lib/server.js |
|
Haven't done thoughtful check, const assert = require('assert')
const WebSocket = require('ws')
const tls = require('tls')
const http = require('http')
const https = require('https')
const http2 = require('http2')
const net = require('net')
function createServer (options, aedesHandler) {
assert(options, 'Missing options')
assert(aedesHandler, 'Missing aedes handler')
var server = null
if (options.serverFactory) {
server = options.serverFactory(aedesHandler, options)
} else if (options.tls) {
server = tls.createServer(options, aaedesHandler)
} else if (options.ws) {
if (options.https) {
if (options.http2) {
server = http2.createSecureServer(options.https)
} else {
server = https.createServer(options.https)
}
} else {
if (options.http2) {
server = http2.createServer()
server.on('session', sessionTimeout(options.http2SessionTimeout))
} else {
server = http.createServer()
}
}
const ws = new WebSocket.Server({ server: server })
ws.on('connection', function (conn, req) {
const stream = WebSocket.createWebSocketStream(conn)
aedesHandler(stream, req)
})
} else {
server = net.createServer(options, aedesHandler)
}
return server
} |
|
@mcollina Are you ok so for a |
|
yes I am |
|
As _socket is not available in ws, so there’s a proposal to create a tiny module to provide that function and replace the websocket-stream, but I don’t think it is bound to aedes only. |
|
@gnought Yes but that requires some modules that maybe the user don't need ? |
|
@mcollina @gnought I would like to work on this in next days but I would like to find something that is good for everone. @mcollina says to create the single websocket module, @gnought prefers a module that will act as server factory for aedes. In my opinion a websocket module is enought as others servers are really easy to setup and that is the only one that requires a little trick What do you think? |
I don't understand what you mean @mcollina. How do you think we should implement protocol decoder so? As an aedes plugin (like it is working now) or like @gnought suggests? As @gnought said now the flow is:
And he would like it to be:
I'm ok for boths but I would like to know what you prefer so we can decide if implement a 'websocket module' or a 'connection factory' module |
|
Decoding of PROXY must happen before decoding the websocket protocol. From my understanding the approach @gnought is suggesting will not work with the most common proxies (haproxy for example). |
|
I hope that the code will be submitted as soon as possible and provide examples for use. If you can't use an API similar to websocket stream on the server, what is the function of the stream API you provide? However, I can only choose websocket stream at present |
@mmm8955405 You can replace const { createWebSocketStream, Server } = require('ws')
function createWebsocketServer (server, broker) {
const ws = new Server({ server })
ws.on('connection', function (conn, req) {
var stream = createWebSocketStream(conn)
stream._socket = conn._socket
broker.handle(stream, req)
})
}
const http = require('http')
const server = http.createServer()
// Than use this function to bind aedes to your server
createWebsocketServer(server, broker) |
|
@mcollina I am suggesting like the following:
The connection factory is like a layer on top of aedes, if we want ws, I will try to create a |
I do not have anything to add to the above. PROXY should not be implemented as a chain of streams. |
|
I need to know what @gnought wants to do so |
|
sorry, too much work on my shoulder. |
|
Hello, I found an issue after upgrading from 0.40 to 0.42. Typescript fails with the following example found in README const aedes = require('./aedes')()
const httpServer = require('http').createServer()
const ws = require('websocket-stream')
const port = 8888
ws.createServer({ server: httpServer }, aedes.handle) // Argument of type '(stream: Connection) => Client' is not assignable to parameter of type '() => void'.ts(2345)
httpServer.listen(port, function () {
console.log('websocket server listening on port ', port)
})Any ideas on how to fix this? does that handle return a Client or void as expected by websocket-stream? |
|
@mcollina This ended up with nothing done. I would like to sort this out but I need to know what in your opinion is the best approach. Actually we use this approach: const { createWebSocketStream, Server } = require('ws')
function createWebsocketServer (server, broker) {
const ws = new Server({ server })
ws.on('connection', function (conn, req) {
// PROTOCOL DECODE SHOULD BE DONE HERE ?
var stream = createWebSocketStream(conn)
stream._socket = conn._socket
broker.handle(stream, req)
})
}
const http = require('http')
const server = http.createServer()
// Than use this function to bind aedes to your server
createWebsocketServer(server, broker)So actually we firstly create the websocket strem then we pass it to aedes handle and here we decode the protocol (proxy or what else) and here we pass the decoded data to mqtt parser. So for what I have understanded the best option could be a |
|
I think so, yes. |
|
@getlarge Would you like to contribute to that? I can init the repos than there should be some tests to do with your protocol decoder |
|
@robertsLando Sure, i could give it a try next week. |
|
@getlarge Nice! Will start working to this next week: https://github.com/moscajs/aedes-server-factory |
|
@robertsLando Just to be clear what's the plan ? As for moving the protocol decoder step in that module, even though i agree, it seems quite tricky as we need to use the same conn stream that aedes is using to extract the information. Eventually we could use a transform stream in between (before aedes.handle is called) to get the connection infos and remove the proxyprotocol header from the stream, but then where the information about the socket and / or proxy would be stored ? Should they be returned by the serverFactory and that becomes user responsability ? And btw, just to add info about the PROXY protocol, it really make sense when we use simple TCP connections (with net module for example), and we need to retrieve information about the |
Yes, that's the main idea.
I started implementing the server factory and I was having the same questions. I'm stucked and I need some ideas... So the main reason of serverFactory would be to simplify the process of creating and attacing aedes to a server and also decode proxy or whatelse, the problem here is that with the actual protocol decoder implementation you had the client instance to attach the connection details and now you don't. I'm stucked 😕 @mcollina Ideas? |
|
@robertsLando The client presence in the arguments of protocolDecoder is one thing that we can workaround quite easily by letting the user, append the extracted data her/himself. I tried something for the serverFactory, it's probably not working but it gives an idea const WebSocket = require('ws');
const tls = require('tls');
const http = require('http');
const https = require('https');
const http2 = require('http2');
const net = require('net');
const net = require('net');
const protocolDecoder = require('aedes-protocol-decoder');
const { Transform, PassThrough } = require('stream');
function createServer(options, aedesHandler) {
return new Promise((resolve, reject) => {
if (!options) {
return reject(new Error('Missing options'));
} else if (!aedesHandler) {
return reject(new Error('aedes handler'));
}
var server = null;
if (options.serverFactory) {
server = options.serverFactory(aedesHandler, options);
resolve({ server });
} else if (options.tls) {
server = tls.createServer(options, aaedesHandler);
resolve({ server });
} else if (options.ws) {
if (options.https) {
if (options.http2) {
server = http2.createSecureServer(options.https);
} else {
server = https.createServer(options.https);
}
} else {
if (options.http2) {
server = http2.createServer();
server.on('session', sessionTimeout(options.http2SessionTimeout));
} else {
server = http.createServer();
}
}
const ws = new WebSocket.Server({ server });
ws.on('connection', function (conn, req) {
const stream = WebSocket.createWebSocketStream(conn);
stream._socket = conn._socket;
// extract proxy if any ?
// const removeProxy = removeFromStream(proxyLength);
// stream.pipe(removeProxy)
aedesHandler(stream, req);
resolve({ server });
});
} else {
server = net.createServer(options, (conn) => {
if (options.useProxy) {
const rewindableStream = conn.pipe(new Rewindable());
const { buf, protocol } = extractProxy(rewindableStream);
const removeProxy = removeFromStream(buf.length);
const rewound = rewindableStream.rewind();
conn = rewound.pipe(removeProxy);
}
aedesHandler(conn);
resolve({ server });
});
}
});
}
const extractProxy = (conn, req) => {
const buf = conn.read(null);
const protocol = protocolDecoder({ req }, buf);
// proto contains {data, ipAddress, port, serverIpAddress, isProxy}
return { buf, protocol };
};
class Rewindable extends Transform {
constructor() {
super();
this.accumulator = [];
}
_transform(buf, enc, cb) {
this.accumulator.push(buf);
callback();
}
rewind() {
const stream = new PassThrough();
this.accumulator.forEach((chunk) => stream.write(chunk));
return stream;
}
}
const removeFromStream = (length) => {
let buffer = Buffer.alloc(0);
let removed = false;
return new Transform({
transform(chunk, encoding, callback) {
if (!removed || buffer.length <= length) {
buffer = Buffer.concat([buffer, chunk], buffer.length + chunk.length);
const partChunk = buffer.slice(length);
this.push(partChunk);
removed = true;
} else {
this.push(chunk);
}
callback();
},
});
};
module.exports = {
createServer,
}; |
|
It seems ok but also a little bit tricky and maybe performance killer? Also If you check here the protocol decoder was running only in case connack wasn't sent, is there any reason to do it on every rquest? |
|
It's based on Promise, so that we can return the server + the connDetails to the user.
The advantage is that aedes would not have to bother with protocol decoding and that would allow to remove those ugly nested conditions in |
|
Previously we needed that connack check because nextBatch was executed for each incoming packet, in the serverFactory, it would be executed only on connection event |
|
as long as it does not slow performance, I'm good. |
|
@robertsLando Yes i can make a PR soon, but i'll have to change the protocolDecoder implementation first, and yes for the org, i will not have not to handle some forks anymore ;) @mcollina For the perf, the connection handling will most probably slower if there is a proxy header because of the stream manipulation, but it could only affect positively the common operations ( incoming messages handling ), as we could remove the nested conditions in aedes.client.nextBatch. |
No description provided.