nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.82k stars 307 forks source link

Websocket test fixes #198

Closed jessie-murray closed 2 years ago

jessie-murray commented 2 years ago

This is a long PR attempting to resolve the issues with Websocket tests, as we've discussed in the past. It is best read commit by commit, rather than as a single diff. I started by cleaning up the websocket test client and ended up rewriting a large part of it to use libevent buffers, which made it possible for me to actually understand what was happening and to manage the read and write buffers in the event-driven context.

I then focused on the webdis side of websockets, by adding trace logs everywhere in src/websocket.c. With the current implementation I don't think they should cause performance problems. These helped me during debugging but now I am less sure of whether they are really needed or not.

While I was fixing this code I also noticed that the HTML page for testing websockets did not have error handling or much styling so I added some CSS and cleaned up the code, it looks prettier now:

image

One thing I was not too sure about was whether the websocket test should be included in the GitHub Action that runs on each pull request, but since this is a lot of new test code maybe it should't be part of the validation yet.

@nicolasff this is my largest change made to webdis so far, I hope it's not too long to review. If this is a problem it could probably be split into several smaller pull requests.

nicolasff commented 2 years ago

Thanks again @jessie-murray! This is pretty long, but I'll try to go over it this weekend.

Just from a quick look, it doesn't seem like there are tests for pub/sub over websockets. If I remember correctly this was really the one feature that had most of the issues; basic websockets worked but the combination of the two would lead to either crashes or hung clients, I don't really remember since it's been so long. Don't get me wrong this already looks like a great start, but this case would need to be covered before the feature can be enabled by default.

jessie-murray commented 2 years ago

@nicolasff the new pub-sub test fails all the time in GitHub actions, I don't think this is due to the WebSocket changes.

jessie-murray commented 2 years ago

There is one major issue I found with the server-side WS code: the handshake is sent back to the client in ws_handshake_reply with just a single write() call: https://github.com/nicolasff/webdis/blob/master/src/websocket.c#L162:

    /* send data to client */
    ret = write(c->fd, buffer, sz);
    (void)ret;
    free(buffer);

    return 0;
}

This needs to be added to a buffer and handled in as many chunks as it takes by the event loop.

jessie-murray commented 2 years ago

Regarding this earlier comment, I made a change to fix it in 8f70db1 and verified it using Valgrind (no memory is leaked or accessed improperly).

jessie-murray commented 2 years ago

@nicolasff Thanks for the first review! I think I've either replied to or made changes to address all your comments, and I no longer have any changes left on my TODO list. As for the long list of commits, I know it's a lot so let me know if you want me to rebase them.

jessie-murray commented 2 years ago

@nicolasff All changes are in, I'm going to stop now for the day but I think the PR looks better now. I believe the WebSocket test is now good enough to add to the "build-and-test" GitHub Action now, although maybe it could go in once the feature is enabled by default.