Skip to content

fix: ensure a single web client connected for debugging #4396

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

Merged
merged 1 commit into from
Feb 26, 2019

Conversation

DimitarTachev
Copy link
Contributor

@DimitarTachev DimitarTachev commented Feb 26, 2019

PR Checklist

connected to: #4301

@cla-bot cla-bot bot added the cla: yes label Feb 26, 2019
@DimitarTachev DimitarTachev modified the milestone: 5.2.1 Feb 26, 2019
Copy link
Contributor

@rosen-vladimirov rosen-vladimirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After fixing comments

@ghost ghost assigned DimitarTachev Feb 26, 2019
@ghost ghost added the new PR label Feb 26, 2019
this.cachedSockets[appId].on("close", () => {
this.destroyDebugSocket(appId);
this.cachedSockets[appId].on("close", async () => {
await this.destroyDebugSocket(appId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This callback will not be awaited and the lock might be released too early. Maybe just resolve a promise outside of the event handler scope after the callback is called and the result of this.destroyDebugSocket(appId) is resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be an infinite wait if we wait for the close event in the create socket method. We are attaching here in order to clean up the cache when the socket is closed.

@DimitarTachev DimitarTachev merged commit 926957a into release Feb 26, 2019
@DimitarTachev DimitarTachev deleted the tachev/unify-debug-ios branch February 26, 2019 12:46
@ghost ghost removed the new PR label Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants