test: add test for async sessionStorageProvider #370
Conversation
test/node/session-provider-async.js
Outdated
| const TIMEOUT = 100; | ||
| const map = new Map(); | ||
| const sessionStorageProvider = { | ||
| set: (sessionId, session) => { |
There was a problem hiding this comment.
Now that I've seen this, I thought about this async set. Ain't there a race condition between sessionStorageProvider.set and user reconnect. So if server uses async sessionStorageProvider and user reconnects too fast he may actually not get a session or he will get an outdated (because the session wasn't saved yet) session, wdyt @nechaido @belochub ?
One possible solution may be adding a callback argument to set and only this._clientsBySessionId.delete(connection.session.id); in the server if we know that it was stored (after callback call).
Also, I assume this race condition is a reason behind https://github.com/metarhia/jstp/pull/370/files#diff-b51f83d123e11290f6de4c2ee483762cR71 ? On a second thought, you need that to test the sessionProvider itself and not session caching. Though point still stands.
Edit: not related to the PR, just smth I thought about.
test/node/session-provider-async.js
Outdated
| const application = new jstp.Application(APP_NAME, interfaces); | ||
|
|
||
| const TIMEOUT = 100; | ||
| const map = new Map(); |
There was a problem hiding this comment.
Put map inside sessionStorageProvider and use function/this? Just a nit.
9c0e4d0 to
2b8efb7
Compare
PR-URL: #370 Reviewed-By: Denys Otrishko <shishugi@gmail.com>
|
Landed in 0b77237. |
PR-URL: #370 Reviewed-By: Denys Otrishko <shishugi@gmail.com>
No description provided.