Skip to content

Ensure websockets write all data; Always keep_alive every websocket #178

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.

Sign up for GitHub

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

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

Ensure websockets write all data; Always keep_alive every websocket #178

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f71bf7
Ensure websockets write all data
shawwn Aug 13, 2020
ce36123
Always keep_alive every websocket
shawwn Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions src/websocket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,26 @@ ws_compute_handshake(struct http_client *c, char *out, size_t *out_sz) {
return 0;
}


void
ws_write_all(int socket, void *buffer, size_t length) {
Copy link
Owner

@nicolasff nicolasff Aug 13, 2020

Choose a reason for hiding this comment

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

Since the caller of this function uses buffer as a char *, you can use the same type here – or even better – const char *. You can also remove the cast to (char*) in the call to write(2).

(minor) file descriptors are named fd throughout the code base, can you please rename socket to fd for consistency? Not to mention that socket is also a function.

size_t written = 0;
int n;
while (written < length) {
int n = write(socket, (char*)buffer + written, length - written);
if (n <= 0) {
if (errno == EINTR || errno == EAGAIN)
continue;
err(EXIT_FAILURE, "Could not write() data");
Copy link
Owner

@nicolasff nicolasff Aug 13, 2020

Choose a reason for hiding this comment

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

This should use the logging interface instead of terminating the service with err. You can pass a pointer to the server struct from the caller and use it like this:

#include "slog.h"

// ...

int
ws_write_all(struct server *s, int fd, void *buffer, size_t length) {

// ...

const char err_msg[] = "Could not write data to websocket";
slog(s, WEBDIS_ERROR, err_msg, sizeof(err_msg)-1);
return -1;

Instead of calling err, a return code should be used to tell the caller about the failure so that it can cleanly terminate the connection and free the WS structure.
Successful writes should return 0; after the while loop. See comment below too.

Unfortunately I see that even if this error is propagated from ws_write_all to ws_handshake_reply to the caller of ws_handshake_reply, the return code is unused in worker.c. There, it should instead be used to set c->broken = 1 just like a couple of lines below the call to ws_handshake_reply.

Something like this. Instead of:

if(nparsed < ret) {
	http_client_add_to_body(c, c->buffer + nparsed + 1, c->sz - nparsed - 1);
	ws_handshake_reply(c);
}

you'd want to use the return value:

if(nparsed < ret) {
	http_client_add_to_body(c, c->buffer + nparsed + 1, c->sz - nparsed - 1);
	if(ws_handshake_reply(c) < 0)
		c->broken = 1;
}

I'm also not sure about this repeated write outside of the event loop. Ideally the call to write(2) should happen when the file descriptor is writable, and the event re-registered if there's still more data to write. This is a more complex change though, and could be done in a second pass.

}
written += n;
}
}


int
ws_handshake_reply(struct http_client *c) {

int ret;
char sha1_handshake[40];
char *buffer = NULL, *p;
const char *origin = NULL, *host = NULL;
Expand Down Expand Up @@ -159,8 +175,7 @@ ws_handshake_reply(struct http_client *c) {
p += sizeof(template4)-1;

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

@nicolasff nicolasff Aug 13, 2020

Choose a reason for hiding this comment

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

The return value of this function should be propagated to the caller in worker.c. Can you put back the int ret; and use it?

Also, to pass a server struct to be used in slog, change to:

ret = ws_write_all(c->s, c->fd, buffer, sz);

(also see comment below)

free(buffer);

return 0;
Copy link
Owner

@nicolasff nicolasff Aug 13, 2020

Choose a reason for hiding this comment

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

Here we'd use ret:

return ret;

Expand Down Expand Up @@ -365,9 +380,7 @@ ws_reply(struct cmd *cmd, const char *p, size_t sz) {

/* send WS frame */
r = http_response_init(cmd->w, 0, NULL);
if (cmd_is_subscribe(cmd)) {
r->keep_alive = 1;
}
r->keep_alive = 1;

if (r == NULL)
return -1;
Expand Down