Skip to content
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

Process killer feature #838

Merged
merged 7 commits into from
Aug 28, 2023
Merged

Process killer feature #838

merged 7 commits into from
Aug 28, 2023

Conversation

udarrr
Copy link
Member

@udarrr udarrr commented Aug 26, 2023

@christian-bromann Generally it's a good feature when we can check and kill selenium processes by port :4444 and drivers names before starting.

Tests added

Bug is here #599

And I'm not convinced but maybe it have sense to be switched on in config by default. Currently it's not switched on.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

I am not sure how useful this would be for a user given they might not know on which port the driver was run. Can't we just improve how we kill the process and try another attempt if we we detect that the port is still occupied?

I think it would be a better approach to actually guarantee to free up the port.

docs/API.md Outdated Show resolved Hide resolved
@udarrr
Copy link
Member Author

udarrr commented Aug 27, 2023

I am not sure how useful this would be for a user given they might not know on which port the driver was run. Can't we just improve how we kill the process and try another attempt if we we detect that the port is still occupied?

I think it would be a better approach to actually guarantee to free up the port.

@christian-bromann Yes agreed it has sense, not necessary kill it if port is free already, and we can take port number from seleniumStatusUrl.port, not necessary set it directly, i've added changes

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

It still is a bit confusing to me why I would need to set opts.processKiller because as mentioned in the comment as a user I would expect that this package is able to shut down the selenium server process and clear the port automatically.

@@ -111,6 +111,8 @@ By default all drivers are loaded, you only control and change the versions or a

`opts.requestOpts` can be any valid [`got` options object](https://www.npmjs.com/package/got#proxies). You can use this for example to set a timeout.

`opts.processKiller` set for that truthy value, for killing selenium server port.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`opts.processKiller` set for that truthy value, for killing selenium server port.
`opts.processKiller` set to a truthy value, for killing selenium server port.

@@ -121,6 +123,8 @@ If you're getting this error, it means that you didn't shut down the server succ
pkill -f selenium-standalone
```

or use truthy `opts.processKiller` in config
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
or use truthy `opts.processKiller` in config
or use `opts.processKiller` in config

@@ -49,6 +49,9 @@ selenium-standalone start --singleDriverStart=chrome
selenium-standalone install --config=/path/to/config.json
selenium-standalone start --config=./config/seleniumConfig.js

# killing process before starting
selenium-standalone start --processKiller=true
Copy link
Member

Choose a reason for hiding this comment

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

A user would ask why they would need to set this given it feels like the expected behavior is that the selenium server or driver shuts down completely when the process is triggered to get shutdown.

@udarrr
Copy link
Member Author

udarrr commented Aug 28, 2023

It still is a bit confusing to me why I would need to set opts.processKiller because as mentioned in the comment as a user I would expect that this package is able to shut down the selenium server process and clear the port automatically.

Yes it could be behavior by default, i've asked about that in first message of the pr. But in the case i'm convinced we should have possibility to switch off it. Because there could be some rare cases when process on port :4444 related to java in list of processes and we're killing java process too. It's necessary when fkill can't perform killing by port and then we'are looking for all processes related to selenium port and killing it. For the most cases it should work but we don't know what else has been connected to java and maybe user wouldn't like to have the behavior.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Ok, sgtm 👍

@christian-bromann christian-bromann merged commit e6135ab into webdriverio:main Aug 28, 2023
8 checks passed
@udarrr udarrr mentioned this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants