0.15 fix async/await and code quality #540
Conversation
|
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. |
| * Subscribe to remote `handleCreateRoom` calls. | ||
| */ | ||
| subscribeIPC(presence, processId, getProcessChannel(), (_, args) => { | ||
| await subscribeIPC(presence, processId, getProcessChannel(), (_, args) => { |
There was a problem hiding this comment.
what is the point of awaiting this? Is there any reason to delay the other calls?
| */ | ||
| export async function query(conditions: Partial<IRoomListingData> = {}) { | ||
| return await driver.find(conditions); | ||
| export function query(conditions: Partial<IRoomListingData> = {}) { |
There was a problem hiding this comment.
this is a good improvement to make. The implementation is much better without extra layers of promises
| roomQuery.sort(handler.sortOptions); | ||
| } | ||
|
|
||
| return await roomQuery; |
There was a problem hiding this comment.
if the function is awaited by the caller, then makes more sense to just remove the async and not await here
|
|
||
| if (!isDevMode) { | ||
| cleanupStaleRooms(name); | ||
| await cleanupStaleRooms(name); |
There was a problem hiding this comment.
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.
|
|
||
| if (!isDevMode) { | ||
| cleanupStaleRooms(name); | ||
| await cleanupStaleRooms(name); |
There was a problem hiding this comment.
seems unnecessary. Cleaning up stale rooms should be handled asynchronously.
I think using .catch might be useful, but why wait?
|
|
||
| // remove processId from room count key | ||
| presence.hdel(getRoomCountKey(), processId); | ||
| await presence.hdel(getRoomCountKey(), processId); |
There was a problem hiding this comment.
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.
| // decrease amount of rooms this process is handling | ||
| if (!isGracefullyShuttingDown) { | ||
| presence.hincrby(getRoomCountKey(), processId, -1); | ||
| await presence.hincrby(getRoomCountKey(), processId, -1); |
There was a problem hiding this comment.
awaiting while shutting down should be avoided
|
|
||
| // unsubscribe from remote connections | ||
| presence.unsubscribe(getRoomChannel(room.roomId)); | ||
| await presence.unsubscribe(getRoomChannel(room.roomId)); |
There was a problem hiding this comment.
these seem unnecessary.
| this._reconnectingSessionId.delete(sessionId); | ||
| }; | ||
|
|
||
| reconnection |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| private _onMessage(client: Client, bytes: number[]) { | ||
| private async _onMessage(client: Client, bytes: number[]) { |
There was a problem hiding this comment.
onMessage should not be awaited
Hi, @endel
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.
tslintconfig from project.onLeavemethod_reserveSeatfromRoom.allowReconnection, so method_reserveSeatgot some changes.Some example of modified
allowReconnectionmethodAllow reconnection with timeout
Allow reconnection with manual cancelation