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

Enforce room ID uniqueness #467

Open
jldinh opened this issue Oct 29, 2021 · 9 comments
Open

Enforce room ID uniqueness #467

jldinh opened this issue Oct 29, 2021 · 9 comments

Comments

@jldinh
Copy link

jldinh commented Oct 29, 2021

Steps to Reproduce (for bugs)

  1. Setup a colyseus cluster with 2 servers and a colyseus proxy (Redis presence and Mongo driver)
  2. Create a room with a filter, inspired by step 1 of https://docs.colyseus.io/colyseus/how-to/password-protect-room/
gameServer
  .define("battle", BattleRoom)
  .filterBy(['password'])
  1. In the onCreate of BattleRoom, assign a custom roomId
async onCreate(options: any) {
  this.roomId = options.password;
}
  1. Use joinOrCreate simultaneously from many clients, with the same password
  2. Multiple rooms with the same ID get created, which is at least a problem for the client method joinBy. It seems clients still get split into multiple rooms even if I sort the rooms by ascending creation date.

Context

I was trying to create rooms with a fixed ID matching an ID provided by another service, but this seems to break joinOrCreate.
This seems to be due to a race condition in the finding of a suitable room, which checks there is no room available, then creates one. If two requests are processed simultaneously, two rooms will be created.
I tried making the roomId index in MongoDB unique, but as the colyseus server did not handle the database errors appropriately, I did not investigate this further.
It seems to me we either need:

  • to query the DB again for the best available room after creating one
  • to make roomIds unique in the DB, and re-query the DB if room creation fails

Your Environment

  • Colyseus Version: 0.14.20
  • Node.js Version: 14.17.4
  • Operating System and version: Ubuntu 20.04
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 29, 2021
@jldinh
Copy link
Author

jldinh commented Nov 29, 2021

bump

@endel endel removed the Stale label Nov 29, 2021
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 30, 2021
@github-actions
Copy link

github-actions bot commented Jan 6, 2022

This issue was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this as completed Jan 6, 2022
@karneaud
Copy link

karneaud commented Jan 9, 2022

bump

@endel endel added this to the 0.16 milestone Jan 10, 2022
@endel
Copy link
Member

endel commented Jan 10, 2022

Hi @karneaud @jldinh, sorry this was closed automatically. I'm assigning a milestone so it won't auto-close again.

Does this happen when roomId is a number, or it doesn't matter if it's a number and/or string?

@endel endel reopened this Jan 10, 2022
@karneaud
Copy link

@endel never tried it with a number but did not work as a string. But I solved my issue by defining/ using another custom property and it seems to work

@jldinh
Copy link
Author

jldinh commented Jan 10, 2022

@endel I also only tried with strings.

@oyed
Copy link
Contributor

oyed commented Jul 24, 2023

I'm doing this via a Redis lock personally, enforcing the acquisition of a lock in onCreate and removing it for onDispose, for example:

async onCreate(options) {
  const myCustomId = options.my_custom_id?.toString();

  if (!myCustomId) {
    throw new Error('No ID provided');
  }

  const obtainedLock = await this.presence.pub.set(`room-${myCustomId}`, 1, 'NX');

  if (obtainedLock !== 'OK') {
    throw new Error('Room already exists');
  }

  this.roomId = myCustomId;

  ...
}

async onDispose() {
  await this.presence.pub.del(`room-${this.roomId}`);
}

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

No branches or pull requests

4 participants