This repository has been archived by the owner on Jun 10, 2022. It is now read-only.
Improvements to client join process #155
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sorenhan
reviewed
Jan 31, 2019
packages/functional-tests/src/app.ts
Outdated
| @@ -116,44 +116,42 @@ export default class App { | |||
| private async displayFunctionalTestChoices(rootActor: MRESDK.ActorLike): Promise<string> { | |||
| return new Promise<string>((resolve) => { | |||
| let y = 0; | |||
| for (const key in this.testFactories) { | |||
| if (this.testFactories.hasOwnProperty(key)) { | |||
There was a problem hiding this comment.
this.testFactories.hasOwnProperty(key) [](start = 20, length = 38)
I thought this was required, TS-lint complaining if missing?
There was a problem hiding this comment.
It's required with for..in, but not for..of.
sorenhan
reviewed
Jan 31, 2019
| this._conn.off('error', this.disconnect); | ||
| this._conn.close(); | ||
| this.emit('close'); | ||
| } catch { } |
There was a problem hiding this comment.
catch { } [](start = 10, length = 9)
should you log error here?
There was a problem hiding this comment.
I'm intentionally not logging errors here because we're in the process of tearing down the session. Various errors can happen depending on how "started up" the session was in the first place. I just want to make sure exceptions don't interrupt the shutdown process from completing.
In reply to: 252832190 [](ancestors = 252832190)
eanders-ms
force-pushed
the
eanders/sync
branch
from
513dfb9
to
0fc7bc4
Compare
This was referenced Jan 31, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changeset started out as an update to the user join process to add protections against zombie sessions. Along the way I encountered and fixed a few other issues (list below).
Glossary
Zombie session: A session where the authoritative peer has become unresponsive but not disconnected.
Zombie session mitigations:
Handshake new connections before adding to session: When a new WebSocket connection is made we no longer spawn an app session right away. We now run through the handshake process first. Once handshake completes we pass the connection to a session where join-in-progress is initiated. This should filter out the unresponsive new connections reported by @willneedit.
Kick unresponsive peers: Messages sent to the client that expect replies now have timeouts on them. Default value is 30 seconds. This can be disabled by setting ConnectionTimeoutSeconds to zero (helpful when sitting on a breakpoint). When a message timeout triggers, the connection is forcibly closed.
Fixes #104
Other changes:
assets-loadedmessages to the app if sent from the authoritative peer.positionandrotationfromRigidBodyclass.