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

WebSocket memory leak #25070

Open
sergeysolovev opened this issue Aug 16, 2024 · 3 comments
Open

WebSocket memory leak #25070

sergeysolovev opened this issue Aug 16, 2024 · 3 comments
Labels
bug Something isn't working correctly ext/websocket related to the ext/websocket crate

Comments

@sergeysolovev
Copy link

Version: Deno 1.45.5
OS: Amazon Linux or Ubuntu Linux

Steps: start the server and the client scripts, the server mem should quickly go up, stop the client script to disconnect. Because the server has disconnected the socket I expect the memory eventually to go down, but it keeps high for very long time.

Here is the log in my case:

Screenshot 2024-08-16 at 23 35 58

Server: spam a client with numerous messages containing a long string. Notice I don’t check the socket’s bufferedAmount field.

const LONG_STRING = 'x'.repeat(1024 * 1024);
const handlers: ClientHandler[] = [];

setInterval(() => {
  const mem = (Deno.memoryUsage().rss / 1024 / 1024).toFixed();
  console.log('srv_stat', { mem });
}, 2500);

Deno.serve({ port: 3333 }, (req) => {
  if (req.headers.get('upgrade') != 'websocket') {
    return new Response(null, { status: 501 });
  }

  const wsUpgrade = Deno.upgradeWebSocket(req);
  handlers.push(new ClientHandler(wsUpgrade.socket));

  return wsUpgrade.response;
});

class ClientHandler {
  socket: WebSocket | null;
  isSubscribed = false;

  constructor(socket: WebSocket) {
    this.socket = socket;
    this.socket.addEventListener('close', this.handleClose);
    this.socket.addEventListener('error', this.handleError);
    this.socket.addEventListener('open', this.handleOpen);
    this.socket.addEventListener('message', this.handleMessage);
  }

  close() {
    this.socket?.close();
    this.socket = null;
  }

  async runSendLoop() {
    while (true) {
      if (this.socket === null) break;
      if (this.socket.readyState === WebSocket.CLOSED) break;
      if (this.socket.readyState === WebSocket.CLOSING) break;

      for (let i = 0; i < 10; i++) {
        this.socket.send(LONG_STRING);
      }

      await this.delay(0);
    }

    console.log('closed_send_loop');
  }

  delay(ms: number) {
    return new Promise((resolve) => setTimeout(resolve, ms));
  }

  handleClose = () => {
    console.log('ws_close');
    if (this.socket === null) return;

    this.socket.removeEventListener('close', this.handleClose);
    this.socket.removeEventListener('error', this.handleError);
    this.socket.removeEventListener('open', this.handleOpen);
    this.socket.removeEventListener('message', this.handleMessage);
    this.socket = null;
  };

  handleOpen = () => {
    console.log('ws_open');
  };

  handleError = (_ev: Event) => {
    const errEv = _ev as unknown as ErrorEvent;
    console.log('ws_err', errEv.message);
  };

  handleMessage = (ev: MessageEvent) => {
    const data = ev.data as string;

    if (data === 'subscribe' && !this.isSubscribed) {
      this.isSubscribed = true;
      console.log('subscribed');
      this.runSendLoop();
      return;
    }
  };
}

Deno.addSignalListener('SIGINT', () => {
  for (const handler of handlers) {
    handler.close;
  }
  Deno.exit();
});

Client: very simple, just send a "subscribe” string and keep receiving the messages from the server

class Client {
  socket?: WebSocket;
  msgCount = 0;
  msgTotLen = 0;

  handleOpen = () => {
    console.log('ws_open');
    this.socket!.send('subscribe');
  };

  handleClose = () => {
    console.log('ws_close');
  };

  handleMessage = (ev: MessageEvent) => {
    this.msgCount += 1;
    this.msgTotLen += (ev.data as string).length;
  };

  handleError = (e: Event) => {
    const errEvent = e as unknown as ErrorEvent;
    console.log('ws_err', errEvent.message);
  };

  open() {
    this.socket = new WebSocket('http://localhost:3333');
    this.socket.addEventListener('open', this.handleOpen);
    this.socket.addEventListener('close', this.handleClose);
    this.socket.addEventListener('error', this.handleError);
    this.socket.addEventListener('message', this.handleMessage);
  }

  close() {
    this.socket?.close();
  }
}

const client = new Client();
client.open();

Deno.addSignalListener('SIGINT', () => {
  client.close();
  setTimeout(() => Deno.exit(), 1000);
});

setInterval(() => {
  const mem = (Deno.memoryUsage().rss / 1024 / 1024).toFixed();
  console.log('stats', {
    mem,
    cnt: client.msgCount,
    len: (client.msgTotLen / 1e6).toFixed(1) + 'm',
  });
}, 2500);
@littledivy
Copy link
Member

This is expected.

while (true) {
  // ...
  for (let i = 0; i < 10; i++) {
    this.socket.send(LONG_STRING);
  }

  await this.delay(0);
}

The code is in a loop and filling up the write queue with lots of data very quickly.

The WebSocket API by design does not provide async backpressure (WebSocketStream fixes this) so its the caller that needs to check socket.bufferedAmount.

@littledivy littledivy added working as designed this is working as intended ext/websocket related to the ext/websocket crate labels Aug 16, 2024
@sergeysolovev
Copy link
Author

Hey @littledivy yes I agree with your comment and even mentioned that in my original message. So yes I expect the memory to grow. What I don’t expect is that some (quite big part part) of the allocated memory stays there even after the connection is closed on the both sides.

@littledivy littledivy added bug Something isn't working correctly and removed working as designed this is working as intended labels Aug 17, 2024
@lucacasonato
Copy link
Member

I do not think this is a memory leak. This is V8 keeping around memory until the system needs it for other reasons - unless your program will eventually crash with an OOM, it is not a leak. V8 does not immediately release memory that JS is not currently using to the OS. It keeps it for a while in case JS needs to alloc in that memory space again. It's a relatively important performance optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly ext/websocket related to the ext/websocket crate
Projects
None yet
Development

No branches or pull requests

3 participants