Skip to content

Fix ArrayBuffer detached error with uWebSockets.js #5331

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ruochenjia
Copy link

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behavior

When using engine.io server with uWebSockets.js, the message buffer becomes detached after executing some async tasks, and throws TypeError when trying to access the buffer later on.

import uWebSocket from "uWebSockets.js";
import { uServer as Engine } from "engine.io";

const httpServer = uWebSocket.App().any("/*", (res, req) => {
	// handle regular HTTP request
}).listen("0.0.0.0", 808080, () => {
	console.log("HTTP server started!");
});

const eio = new Engine({
	transports: ["polling", "websocket"]
});

eio.on("connection", (socket) => {
	socket.on("close", () => {
		console.log("Socket closed");
	});
	socket.on("message", (buffer) => {
		console.log(buffer.toString()); // ok here

		someAsyncTask().then(() => {
			console.log(buffer.subarray(0, 3).toString()); // TypeError: Cannot perform Construct on a detached ArrayBuffer
		});
	});
});

eio.attach(httpServer);

New behavior

The original ArrayBuffer passed by the uWebSockets.js is always copied to prevent it being detached. This matches the behavior for a regular engine.io server using the built-in http module.

The documentation of uWebSockets.js states that for the message handler, the given ArrayBuffer is valid during the lifetime of this callback (until first await or return) and will be neutered.

Other information (e.g. related issues)

@darrachequesne
Copy link
Member

Hi!

I'm not sure that this should be done inside the library, but rather left to the user, depending on its use case (for performance purposes). What do you think?

@ruochenjia
Copy link
Author

@darrachequesne

Executing async functions within a callback is a very common practice in JavaScript. Detaching a buffer after a period of time is not common and usually only happens in the web stream API or when the application runs out of memory. This unusual behavior makes it very hard to debug especially there is no related documentation in the library. Beginners might not even know that ArrayBuffer objects can be detached.

Most uWebSockets.js users used to use the built-in http module with regular engine.io Server, which never detaches ArrayBuffer objects. They switched to uWebSockets.js because of better performance, but having an inconsistent API significantly increases the debugging complexity and wasted time in rewriting all their async code. This would stop many users from migrating the backend, which eventually makes the uServer useless.

Performance is not an issue to consider at all because buffer received in a single message is usually small, which makes the copying process extremely fast especially when the slice method is implemented in native code and Node.js has optimizations for it. The library already creates overhead while encoding and decoding messages compared to using plain websockets, so the overhead caused by copying a small buffer is just like creating objects using literals, which is negligible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants