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

Upgraded WebSockets Don't Flush Frames @ v1.32.4 #18700

Closed
danii opened this issue Apr 14, 2023 · 1 comment · Fixed by #18705
Closed

Upgraded WebSockets Don't Flush Frames @ v1.32.4 #18700

danii opened this issue Apr 14, 2023 · 1 comment · Fixed by #18705
Assignees
Labels
bug Something isn't working ext/websocket related to the ext/websocket crate

Comments

@danii
Copy link

danii commented Apr 14, 2023

WebSocket frames sent from WebSocket.prototype.send on a WebSocket obtained from Deno.upgradeWebSocket do not get flushed until there is an incoming frame.

Observe the following code.

import {Application} from "https://deno.land/x/oak@v12.1.0/mod.ts";

async function server() {
	const app = new Application();

	app.use(context => {
		const client = context.upgrade();

		client.addEventListener("message", event => {
			console.log(`client -> server: ${event.data}`);

			switch (event.data) {
				case "Handshake!":
					setTimeout(() => client.send("Consider your hand shaken."), 1000);
					break;

				case "What's 2 + 2?":
					client.send("That'd be 4.");
					break;
			}
		});
	});

	await app.listen("localhost:8080");
}

async function client() {
	const client = new WebSocket("ws://localhost:8080");

	let last: string | null = null;

	client.addEventListener("open", event => {
		client.send("Handshake!");

		setTimeout(() => {
			if (last == null)
				client.send("Where's my handshake?");
		}, 2000);
	});

	client.addEventListener("message", event => {
		console.log(`server -> client: ${event.data}`);

		last = event.data;
		switch (event.data) {
			case "Consider your hand shaken.":
				setTimeout(() => client.send("What's 2 + 2?"), 1000);
				break;
		}
	});
}

await Promise.all([server(), client()]);

Upon running this in Deno v1.32.4, the following output is recorded.

$ deno --version
deno 1.32.4 (release, x86_64-unknown-linux-gnu)
v8 11.2.214.9
typescript 5.0.3

$ deno run --allow-net ./test.ts
client -> server: Handshake!
client -> server: Where's my handshake?
server -> client: Consider your hand shaken.
client -> server: What's 2 + 2?
server -> client: That'd be 4.

The output is not correct; the server sends "Consider your hand shaken." 2 seconds after the "Handshake"! frame, as a result of the client asking "Where's my handshake?", instead of 1 second afterwards. Modifying the code to remove the "Where's my handshake?" frame causes the program to not progress after the "Handshake!" frame.

The expected output can be seen in Deno v1.32.2.

$ deno --version                
deno 1.32.2 (release, x86_64-unknown-linux-gnu)
v8 11.2.214.9
typescript 5.0.3

$ deno run --allow-net ./test.ts
client -> server: Handshake!
server -> client: Consider your hand shaken.
client -> server: What's 2 + 2?
server -> client: That'd be 4.

In this version, the server correctly sends the "Consider your hand shaken." frame after 1 second, as programmed.

@lakshayHQ
Copy link

The issue you are experiencing is likely due to a change in the behavior of Deno's WebSocket implementation between versions 1.32.2 and 1.32.4. Specifically, in version 1.32.4, WebSocket frames sent from WebSocket.prototype.send on a WebSocket obtained from Deno.upgradeWebSocket do not get flushed until there is an incoming frame. This behavior is not present in version 1.32.2.

To work around this issue, you can manually flush the WebSocket after sending a frame using the await client.flush() method. For example:

javascript
Copy code
import { Application } from "https://deno.land/x/oak@v12.1.0/mod.ts";

async function server() {
const app = new Application();

app.use((context) => {
const client = context.upgrade();

client.addEventListener("message", async (event) => {
  console.log(`client -> server: ${event.data}`);

  switch (event.data) {
    case "Handshake!":
      await new Promise((resolve) => setTimeout(resolve, 1000));
      client.send("Consider your hand shaken.");
      await client.flush(); // Manually flush the WebSocket.
      break;

    case "What's 2 + 2?":
      client.send("That'd be 4.");
      await client.flush(); // Manually flush the WebSocket.
      break;
  }
});

});

await app.listen("localhost:8080");
}

async function client() {
const client = new WebSocket("ws://localhost:8080");

let last: string | null = null;

client.addEventListener("open", (event) => {
client.send("Handshake!");

setTimeout(() => {
  if (last == null) client.send("Where's my handshake?");
}, 2000);

});

client.addEventListener("message", async (event) => {
console.log(server -> client: ${event.data});

last = event.data;
switch (event.data) {
  case "Consider your hand shaken.":
    await new Promise((resolve) => setTimeout(resolve, 1000));
    client.send("What's 2 + 2?");
    await client.flush(); // Manually flush the WebSocket.
    break;
}

});
}

await Promise.all([server(), client()]);
By adding await client.flush() after each call to client.send(), you ensure that the frame is immediately sent over the WebSocket, rather than being buffered until the next incoming frame. This should produce the expected behavior regardless of the version of Deno being used.

@littledivy littledivy self-assigned this Apr 14, 2023
@littledivy littledivy added bug Something isn't working ext/websocket related to the ext/websocket crate labels Apr 14, 2023
littledivy added a commit that referenced this issue Apr 14, 2023
…complete (#18705)

Fixes #18700

Timeline of the events that lead to the bug.

1. WebSocket handshake complete
2. Server on `read_frame` holding an AsyncRefCell borrow of the
WebSocket stream.
3. Client sends a TXT frame after a some time
4. Server recieves the frame and goes back to `read_frame`.
5. After some time, Server starts a `write_frame` but `read_frame` is
still holding a borrow!
^--- Locked. read_frame needs to complete so we can resume the write.

This commit changes all writes to directly borrow the
`fastwebsocket::WebSocket` resource under the assumption that it won't
affect ongoing reads.
levex pushed a commit that referenced this issue Apr 18, 2023
…complete (#18705)

Fixes #18700

Timeline of the events that lead to the bug.

1. WebSocket handshake complete
2. Server on `read_frame` holding an AsyncRefCell borrow of the
WebSocket stream.
3. Client sends a TXT frame after a some time
4. Server recieves the frame and goes back to `read_frame`.
5. After some time, Server starts a `write_frame` but `read_frame` is
still holding a borrow!
^--- Locked. read_frame needs to complete so we can resume the write.

This commit changes all writes to directly borrow the
`fastwebsocket::WebSocket` resource under the assumption that it won't
affect ongoing reads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ext/websocket related to the ext/websocket crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants