-
Notifications
You must be signed in to change notification settings - Fork 239
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
Add http middleware and websocket upgrade on existing listener #755
base: public
Are you sure you want to change the base?
Add http middleware and websocket upgrade on existing listener #755
Conversation
@wilberforce – this looks very cool. I'm just starting to go thought it. My focus is the integration on the HTTP and WebSocket servers. If I'm reading it correctly, it looks this is what you do
Am I close? |
Yes. Additionally the socket close is then handled by the Middleware |
There is another event stream type that esphome uses for debug output and event status: https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events I think this would also be useful middleware, and thinks like Here is a nodejs implementation server side: |
It would be straightforward to implement server-sent events -- both client and server. I was never that motivated to implement it because WebSockets is is all that and more. But, I suppose for compatibility having server-sent events would be useful. But... that's another topic. This one is big enough already. Let's stay focused. ;) |
I missed that, thanks. In my mental model, only one layer should own the socket at a time. Once the socket has been handed off to WebSockets, then WebSockets should be responsible for closing it. Another layer could take it back using |
Yes - the websocket middleware takes responsibility
Not sure quite what you mean here? |
Only one layer/module should be responsible for the socket at a time. After it is handed off to the WebSocket server, no other module should hold a reference to it. That is the only way to be sure that no problems happen because of unsynchronized access by both. Separately, the |
This is how the connection is managed, once the socket has upgraded
The connection is pushed into a list on the moddable/modules/network/http/middleware/server.js Lines 67 to 69 in 7b60140
Difficult to reuse. The http class already decoded all the headers so that code was not required. If the core parts of the switch were pulled out into methods then they could be re-used. |
Understood. I tried anyway. The result works well and feels straightforward to use. Let's walk through the client code. First, instantiate the servers. The HTTP server is instantiated as usual. SInce the WebSocket server will use connections that come in from HTTP, it doesn't need a listener socket. That is signaled by passing the constructor import {Server as HTTPServer} from "http"
import {Server as WebsocketsServer} from "websocket"
const websockets = new WebsocketsServer({port: null});
websockets.callback = function (message, value) {...};
const http = new HTTPServer({port: 80}); Then, the HTTP server callback needs to detect when a request is made to a Websockets endpoint (path). Here it just checks the path for http.callback = function(message, value, etc)
{
if ((HTTPServer.status === message) && ("/ws" === value)) {
const socket = http.detach(this);
websockets.attach(socket);
return;
}
...
} From there, no more HTTP callbacks are received on the detached connection and the WebSocket callback operates as usual. The WebSockets Time is a little short today, so I'm just attaching the |
Hi, I put the your changes here so I could see the differences easily: It is tidy with the attach/detach. Do you propose changing the middleware to use this websocket implementation? Happy new year... |
(Apologies for going quiet. My GitHub in-box got messed up, destroying one of my to-do lists...)
Cool, thanks.
Agreed. And, I think more generally useful.
Yes. It would also eliminate the need to duplicate a chunk of code from the WebSocket server. Have you thought about other names than "middleware"? I think of "middleware" as a category of software, not a specific implementation. |
I was using the naming convensions from Node's express: https://expressjs.com/en/guide/using-middleware.html as it's using similiar chained techiques, although I'm using a class implemention. |
Following the music theme of pico I thought of using a musical term for middle - the most appropriate seemed to be bridge, Does that work?
|
@phoddie I'm not sure I need the websocket |
I do prefer bridge to middleware. It is reasonably descriptive here. |
I've been putting an example together - pretty happy with it.... I'll move it to examples and post tomorrow, and add a readme for the build steps.... By the way is there are a way of getting |
There's no way to run a script directly. Recipes is a way to modify the build for certain files, but it may not be what you are looking for here. |
@phoddie |
I've done an npm script that builds the zip and then runs |
That's probably the best solution at the moment. I'd like to have the manifest provide a way to create a ZIP file from sources, but that's a bit more work that I've time for right now. Apologies for not getting back to this sooner. I've been distracted elsewhere. I'd like to see this land, so I'll find some time to review. |
It can't find |
This is great example that weaves together many different network tools and techniques frequently used in projects that want to support a web service on the local network. I'm not sure about where it below. A couple observations:
I suggest we start by putting this in One detail about the WebSockets patch. It invokes callbacks directly from within the import Timer from "timer";
//...
export class Client {
//...
close() {
this.socket?.close();
delete this.socket;
if (this.timer)
Timer.clear(this.timer);
delete this.timer;
}
}
//...
export class Server {
#listener;
constructor(dictionary = {}) {
if (null === dictionary.port)
return;
this.#listener = new Listener({port: dictionary.port ?? 80});
this.#listener.callback = () => {
const socket = new Socket({listener: this.#listener});
const request = new Client({socket});
request.doMask = false;
socket.callback = server.bind(request);
request.state = 1; // already connected socket
request.callback = this.callback; // transfer server.callback to request.callback
request.callback(Server.connect, this); // tell app we have a new connection
};
}
close() {
this.#listener?.close();
this.#listener = undefined;
}
attach(socket) {
const request = new Client({socket});
request.doMask = false;
socket.callback = server.bind(request);
request.state = 1; // already connected socket
request.callback = this.callback; // transfer server.callback to request.callback
request.state = 2;
request.flags = 0;
request.timer = Timer.set(() => {
delete request.timer;
request.callback(Server.connect, this); // tell app we have a new connection
socket.callback(2, socket.read());
});
}
}; |
Apologies for the delay. I've done some work on the WebSockets attach implementation. That will be in the next Moddable SDK update later this week. It includes an example and updated docs. Once that lands, please confirm that your contribution still works and then we can merge this PR! Notes in brief:
It isn't hijacking the port, but using a null for the port to signal that no listener needs to be created.
It is. But it is unavoidable in order to maintain the JavaScript convention that callbacks generally should not be invoked from inside a user function call.
Resolved. |
@phoddie I'll take a look at your code when it's posted. |
Curious. FWIW – I just did a quick check and verified that each server request is called with a unique |
In my pull request I did the callback in the constructor - it was pretty clean. Are you able to attach or point me to your websocket.js so I can try it? |
Clean or no, we can't do that: it breaks the rules. |
Here you go. Contains the latest websockets.js and a simple test app |
I changed to use your implementation. stepping: So there is no more data in the socket so the handshake does not finish. This is the code from the bridge websocket:
|
If I add debugging to your websocket code:
So something is consuming the buffer.. ? |
If I make
|
I've gone back to your example. In the console I only see If I take the timeout out it , and do the callback and socket read it works. What is the reason for the callback? |
Let's try to get on the same page. Here's what I just did:
The output in xsbug looks something like this (taken from the ESP32 run): Wi-Fi connected to "My Home Wi-Fi"
IP address 10.0.0.25
ws connect
ws handshake
ws message received: {"hello":"world"}
ws connect
ws handshake
ws message received: {"hello":"world"}
ws connect
ws handshake
ws message received: {"hello":"world"}
ws close
ws connect
ws handshake
ws message received: {"hello":"world"}
ws close
ws close
ws close I believe you are using Windows, so that's different. But, the ESP8266 and ESP32 builds should give the same result. |
I'm not not winning here.
d. I'm still getting the same issue with the windows sim. I've echoed IP and have the socket available bytes:
I don't understand why the timer is required - why can't the callback and socket be read straight away? Thanks |
Thank you for your patience.
That's correct. That is why step 2 above is "Added the websockets.js and httpserverwithwebsockets example from the ZIP above."
That appears unrelated to this topic. The Moddable SDK did recently update to ESP-IDF 4.4. Perhaps you don't have the expected version. In any case, this isn't the issue to solve that.
A synchronous function, such as
This is important because callbacks from within a synchronous function are often unexpected and lead to difficult to track down bugs. It may be that the issue you are seeing is isolated to Windows, perhaps due to a difference in the socket implementation there. Before going down that road further, I think it is important that we are able to reproduce any of the same results. It seems that ESP32 is the place for that. |
I have installed a clean idf 4.4. Helloword builds correctly. Still getting this error: Appears to be related to this: https://esp32.com/viewtopic.php?t=14651 the regedit hack was already set on my machine. I copied the project form: and now its builds. Grrrr The esp32 build works for me:
So it appears to be related to the windows socket implementation in the simulator. |
I can't tell you how much I hate that path length issue. A way to avoid it is to turn off |
Thanks @andycarle and this builds: Any idea why socket is behaving differently in the windows simulator? |
Windows socket should now behave like macOS, ESP32, and ESP8266. Thanks to @andycarle for the debugging help. |
Thanks guys. I can confirm the fix is working for me! I can't say I understand the changes - but it works! Fantastic support. |
Upload.from.GitHub.for.iOS.MOVQuick video showing web sockets updating multiple devices |
It works!! |
Somehow this hasn't been merged yet. I think it is ready to go? @wilberforce - good to go? |
@phoddie |
This PR makes the http webserver extensible by the addition of optional middleware. You can pick and choose what handlers you want to install or write new ones by subclassing
Middleware
.Each handler is called in a chain until a URL match is found, if none found and 500 error is sent.
The attached wifiprovision shows a complete example.