uNetworking / uWebSockets

Simple, secure & standards compliant web server for the most demanding of applications
Apache License 2.0
17.42k stars 1.76k forks source link

Publish not being received by client when unsubscribing [from all topics] in the same tick #1673

Closed myndzi closed 1 year ago

myndzi commented 1 year ago

According to the release notes:

Pub/sub is now a lot more predictable and always guarantees strict ordering inside topics, across topics and between WebSocket::send and WebSocket::publish / App::publish. Subscription and unsubscription is always guaranteed to be strictly ordered with respect to publish. Support for MQTT wildcards has been removed.

I have a case where I'm working on the backend for an existing application; I'm using pub/sub to represent game lobbies. When the host leaves the room, a control message is sent to the topic, and then each subscribed client is unsubscribed. However, the control message is not being sent.

I managed to thin it down to a minimum reproducible version, though my initial attempts seemed to work as expected. I was unable to determine the significant difference between my initial attempts and the repro case below. I've been at it some hours, hopefully this is actually a repro case and not user error.

const uWS = require('uWebSockets.js');
const { WebSocket } = require('ws');

const app = uWS
  .App()
  .ws('/', {
    open: (ws) => {
      ws.subscribe('test');
      app.publish('test', 'hi from pubsub');
      // swap comments on the next two lines - will change between success and failure
      ws.unsubscribe('test');
      // setImmediate(() => ws.unsubscribe('test'));
      ws.send('???');
    }
  })
  .listen(3300, () => {
    const wsClient = new WebSocket('ws://localhost:3300/');
    wsClient.addEventListener('message', (evt) => {
      const msg = Buffer.from(evt.data).toString();
      console.log('client received from server:', msg);
      if (msg === '???') app.close();
    });
  });
$ npm ls
uws-bug@1.0.0 /home/myndzi/uws-bug
├── uWebSockets.js@20.32.0 (git+ssh://git@github.com/uNetworking/uWebSockets.js.git#21c0e41a662c27bcc29b64f3e9d7f0bdaa6d7ee7)
└── ws@8.14.2

$ node --version
v20.8.1
myndzi commented 1 year ago

Note: if the client is subscribed to a different pubsub topic, it receives the result it was intended to. This produces the expected result:

const uWS = require('uWebSockets.js');
const { WebSocket } = require('ws');

const app = uWS
  .App()
  .ws('/', {
    open: (ws) => {
      ws.subscribe('other'); // new
      ws.subscribe('test');
      app.publish('test', 'hi from pubsub');
      ws.unsubscribe('test');
      ws.send('???');
    }
  })
  .listen(3300, () => {
    const wsClient = new WebSocket('ws://localhost:3300/');
    wsClient.addEventListener('message', (evt) => {
      const msg = Buffer.from(evt.data).toString();
      console.log('client received from server:', msg);
      if (msg === '???') app.close();
    });
  });

Edit: come to think of it, this was probably the difference-maker in my attempted reproduction, since I had a bug in my actual code where I forgot to subscribe users to the lobby, so I wasn't getting the control message since the users, once unsubscribed, were present in 0 pubsub topics. But since I had intended them to be in the lobby, my repro code didn't have that bug and worked unexpectedly...

uNetworkingAB commented 1 year ago

https://github.com/uNetworking/uWebSockets/commit/7da88ce71c31dbc799dfa9bdd8171d09a6b80b19

Will release this if it passes fuzzing

myndzi commented 1 year ago

Thank you! One Q: looking at the diff, is there anything that cleans these up? This feels leaky to me, though it will solve the problem. Is there no place to remove the subscriber once all messages have been flushed?

uNetworkingAB commented 1 year ago

freeSubscriber is not per topic, it's more like "downgrade this WebSocket to not using pub/sub". It happens in close either way.

uNetworkingAB commented 1 year ago

In a few hours, can you test bun install uNetworking/uWebSockets.js#binaries with these changes

uNetworkingAB commented 1 year ago

Did that work?

myndzi commented 1 year ago

Did that work?

Yep, thank you!

uNetworkingAB commented 1 year ago

Yeah I ran your reproducer and saw it was fixed. Thanks for reporting, there is a new release both .js and c++