-
-
Notifications
You must be signed in to change notification settings - Fork 412
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
fix: Make sure we close the device proxy if node process gets killed #1153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach seems sound. A couple of comments.
lib/device-connections-factory.js
Outdated
this.localServer.on('error', (e) => this.log.warn(e.message)); | ||
|
||
this.onBeforeProcessExit = () => { | ||
if (this.localServer?.listening) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure that the dependency is set to require at least appium-gulp-plugins@5.1.0
or this won't build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
@KazuCocoa Could you please help debugging this code? |
Okay, will take a look |
https://gist.github.com/KazuCocoa/18be61e5a0860eea24760cd4d5158edb I checked as:
After is this PR. Before is the current master. Exactly |
thanks @KazuCocoa |
@KazuCocoa Would you mind checking appium/appium#13913 instead? |
The case was the same as SIGINT |
Thanks @KazuCocoa |
After eae7a59 ,
note: |
bb615bb
to
92dc78b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7e17810 bursted CLOSE_WAIT
like https://gist.github.com/KazuCocoa/8a7340f01659ee835d9da19c29fef1aa
(and never the connection succeeded)
Thanks for checking that @KazuCocoa |
Okay. Let me take a look further |
note: Current log did not have
|
@@ -137,7 +137,7 @@ class DeviceConnectionsFactory { | |||
promises.push((async () => { | |||
log.info(`Releasing the listener for '${key}'`); | |||
try { | |||
await this._connectionsMapping[key].iproxy.quit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix #1153 (comment)
lib/device-connections-factory.js
Outdated
if (e) { | ||
reject(e); | ||
} else { | ||
this.log.info(`The connection to ${this.udid} has been successfully closed`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else never called, and timeout error happened
After SIGINT/quit, Current implementation worked on my local for SIGING/quit and establish a new session against the same port after it. |
This is great news @KazuCocoa ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup/optimisation after my change!
The latest commit works well :) LGTM (should wait for CI tho)
No description provided.