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.

Sign up for GitHub

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

Jump to bottom

0.15 fix async/await and code quality #540

Open

vitalyrotari wants to merge 11 commits into colyseus:0.15 from vitalyrotari:0.15-fix-async-await

vitalyrotari commented Sep 30, 2022 •

edited

Our project is working more than 1 year on amazing Colyseus, now we made some new feature, and I found many issues with async/await inside of core and other minor things.

I'm not sure if this PR will be merged completely, but they contains many fixes and improvements.

Fix code style based according tslint config from project.
Fix unresolved async/await
Unify Presence interface methods
Unify MatchMakerDriver interface methods
Add ClientRef interface and missing onLeave method
Fix unresolved async _reserveSeat from Room.allowReconnection, so method _reserveSeat got some changes.

Some example of modified allowReconnection method

Allow reconnection with timeout

  try {
    await this.allowReconnection(client, 30);
  } catch (e) {
    // ...
  }

Allow reconnection with manual cancelation

  const controller = new AbortController();
  
  setTimeout(() => {
    controller.abort();
  }, 5000);

  try {
    await this.allowReconnection(client, 'manual', controller);
  } catch (e) {
    // ...
  }

vitalyrotari added 10 commits

September 30, 2022 10:01


          fix code style based on lint rules

106d9a7


          unify presence API

22477a9


          unify driver API

f29d0df


          describe client ref onLeave method

b94fa9e


          fix unresolved async and refactor allowReconnection

a38900e


          make room clients readonly

5a45007


          fix missing async/await

73c76f9


          fix transport client ref typing

f6dc09d


          fix return type for Server.define

26bf15d


          fix async/await for forciblyCloseClient and onMessage

448206c

github-actions bot commented Nov 20, 2022

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions bot added the Stale label

endel removed the Stale label

endel added this to the 0.15 milestone


          Merge branch '0.15' into 0.15-fix-async-await

d3f7db6

hunkydoryrepair reviewed

View reviewed changes

Contributor

hunkydoryrepair left a comment

There is a lot here that will break or slow down existing apps.
Some beneficial changes, too, but I think a smaller PR with more isolated changes would be better.

packages/core/src/MatchMaker.ts

@@ @@ -65,7 +65,7 @@ export async function setup( @@
                 /**
                  * Subscribe to remote `handleCreateRoom` calls.
                  */
-                subscribeIPC(presence, processId, getProcessChannel(), (_, args) => {
+                await subscribeIPC(presence, processId, getProcessChannel(), (_, args) => {

Contributor

hunkydoryrepair Nov 12, 2023

what is the point of awaiting this? Is there any reason to delay the other calls?

packages/core/src/MatchMaker.ts

               /**
                * Perform a query for all cached rooms
                */
-              export async function query(conditions: Partial<IRoomListingData> = {}) {
-                return await driver.find(conditions);
+              export function query(conditions: Partial<IRoomListingData> = {}) {

Contributor

hunkydoryrepair Nov 12, 2023

this is a good improvement to make. The implementation is much better without extra layers of promises

packages/core/src/MatchMaker.ts

                     roomQuery.sort(handler.sortOptions);
                   }
-                  return await roomQuery;

Contributor

hunkydoryrepair Nov 12, 2023

if the function is awaited by the caller, then makes more sense to just remove the async and not await here

packages/core/src/MatchMaker.ts

@@ @@ -245,17 +245,17 @@ export function defineRoomType<T extends Type<Room>>( @@
                 handlers[name] = registeredHandler;
                 if (!isDevMode) {
-                  cleanupStaleRooms(name);
+                  await cleanupStaleRooms(name);

Contributor

hunkydoryrepair Nov 12, 2023

This could be breaking. Inserting asynchronous code into what is currently a syncrhonous call could break some apps, and changing to async changes the return type, so it is definitely app breaking.

packages/core/src/MatchMaker.ts

                 delete handlers[name];
                 if (!isDevMode) {
-                  cleanupStaleRooms(name);
+                  await cleanupStaleRooms(name);

Contributor

hunkydoryrepair Nov 12, 2023

seems unnecessary. Cleaning up stale rooms should be handled asynchronously.
I think using .catch might be useful, but why wait?

packages/core/src/MatchMaker.ts

                 }
                 // remove processId from room count key
-                presence.hdel(getRoomCountKey(), processId);
+                await presence.hdel(getRoomCountKey(), processId);

Contributor

hunkydoryrepair Nov 12, 2023

gracefullyShutdown is called as a result of unhandled exceptions. As soon as you await when processing an unhandled exception, the application will exit, thus, this will leave redis full and won't even call disconnect. This would be a pretty big problem when we want a chance to save data during unhandled exception.

packages/core/src/MatchMaker.ts

@@ @@ -563,7 +565,7 @@ async function disposeRoom(roomName: string, room: Room) { @@
                 // decrease amount of rooms this process is handling
                 if (!isGracefullyShuttingDown) {
-                  presence.hincrby(getRoomCountKey(), processId, -1);
+                  await presence.hincrby(getRoomCountKey(), processId, -1);

Contributor

hunkydoryrepair Nov 12, 2023

awaiting while shutting down should be avoided

packages/core/src/MatchMaker.ts

                 // unsubscribe from remote connections
-                presence.unsubscribe(getRoomChannel(room.roomId));
+                await presence.unsubscribe(getRoomChannel(room.roomId));

Contributor

hunkydoryrepair Nov 12, 2023

these seem unnecessary.

packages/core/src/Room.ts

+                      this._reconnectingSessionId.delete(sessionId);
+                    };
+                    reconnection

Contributor

hunkydoryrepair Nov 12, 2023

I've never been able to get the reconnection aspect to work well with colyseus so I'm not sure of the impact here, but it should still RETURN something. This has been change to have no return value, so the call cannot be awaited anymore

packages/core/src/Room.ts

                   return await (userReturnData || Promise.resolve());
                 }
-                private _onMessage(client: Client, bytes: number[]) {
+                private async _onMessage(client: Client, bytes: number[]) {

Contributor

hunkydoryrepair Nov 12, 2023

onMessage should not be awaited

endel modified the milestones: 0.15, 1.0

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