Closing out logging on quit

[Sean-Gomez]

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

The line I added closes out the log file connections before the service performs a full shutdown

Motivation and Context

A project I'm working on uses the Selenium webdriver and I noticed unclosed sockets remained in some of our unit tests, which could potentially affect performance. This addition should now properly close the open sockets of the log output file process.

Types of changes

• [X ] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[CLAassistant]

All committers have signed the CLA.

[titusfortner]

The biggest problem is that not all log_output instances respond to close(). It can be used to set STDOUT or STDERR which are actually int

What if we move it to the Service class's close() method and put in a conditional so that it works for log file, stdout or null?

[Sean-Gomez]

Hi Titus, my latest commit should close log_output if it's set to an int like you said. Sorry, but I'm not sure what you meant when you said move it to the server class' close method, could you clarify that if there's still an issue with the commit? Thanks!

[titusfortner]

oops, sorry, I meant the stop() method: https://github.com/Sean-Gomez/selenium/blob/ed632c22efccab6abb88de25622d9f40a1de389c/py/selenium/webdriver/common/service.py#L142

This logic would apply to all the service classes, not just Firefox.

[titusfortner]

Ok, I moved your solution to the stop method. There was already logic there, but I don't think it was doing the right thing.

@isaulv can you double check us on this change?

I think the issue is that devnull needs to be closed and we were ignoring it before. Also not sure what previous code was rescuing; should I keep that just in case?

[isaulv]

@titusfortner I think the proposed code is correct. It will close open files (TextIO) and if it's a file descriptor like stdout or stdin, os.close should handle that file descriptor, and even dev/null.
It shouldn't throw if the resource is already closed.

[isaulv]

LGTM

[codecov-commenter]

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.02% ⚠️

Comparison is base (8101236) 57.02% compared to head (f728fa1) 57.01%.
Report is 4 commits behind head on trunk.

❗ Current head f728fa1 differs from pull request most recent head 0eab6cb. Consider uploading reports for the commit 0eab6cb to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #12637      +/-   ##
==========================================
- Coverage   57.02%   57.01%   -0.02%     
==========================================
  Files          86       86              
  Lines        5322     5325       +3     
  Branches      192      192              
==========================================
+ Hits         3035     3036       +1     
- Misses       2095     2097       +2     
  Partials      192      192              

Files Changed	Coverage Δ
py/selenium/webdriver/remote/remote_connection.py	53.87% <0.00%> (-0.71%)	⬇️
py/selenium/webdriver/common/service.py	91.05% <100.00%> (+0.81%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[titusfortner]

Thanks @Sean-Gomez