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.

Sign up for GitHub

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

Jump to bottom

Properly close SSL socket in _loopback_for_cert #291

Merged
merged 5 commits into from Jun 11, 2020
Merged

Properly close SSL socket in _loopback_for_cert #291

merged 5 commits into from Jun 11, 2020

Conversation

mxii-ca
Copy link
Contributor

@mxii-ca mxii-ca commented Jun 8, 2020 •
edited by webknjaz

❓ What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • 📋 docs update
  • 📋 tests/coverage improvement
  • 📋 refactoring
  • 💥 other

📋 What is the related issue number (starting with #)

Fixes #290

❓ What is the current behavior? (You can also link to an open issue here)

From #290 :

With python3 -X dev -X tracemalloc=5 on cheroot code with SSL, python gives a ResourceWarning

❓ What is the new behavior (if this is a feature change)?

No more ResourceWarning

📋 Other information:

While the original code (submitted in #243) closed the underlying loopback socket, it did not properly close the ssl wrappers. This PR fixes that.

This PR also updates the test_ssl_env test to fail if a ResourceWarning was emitted.

📋 Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

This change is Reviewable

webknjaz
webknjaz reviewed Jun 10, 2020
View reviewed changes
cheroot/test/test_ssl.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jun 10, 2020
View reviewed changes
cheroot/test/test_ssl.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jun 10, 2020
View reviewed changes
cheroot/test/test_ssl.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jun 11, 2020
View reviewed changes
cheroot/test/test_ssl.py Outdated Show resolved Hide resolved
@webknjaz webknjaz changed the title Properly close SSL socket in _loopback_for_cert Properly close SSL socket in _loopback_for_cert Jun 11, 2020
webknjaz
webknjaz reviewed Jun 11, 2020
View reviewed changes
cheroot/test/test_ssl.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented Jun 11, 2020 •
edited

Spellcheck job failure looks unrelated: sphinx-contrib/spelling#49 sphinx-doc/sphinx#7802.

@webknjaz webknjaz merged commit 7e49940 into cherrypy:master Jun 11, 2020
@webknjaz
Copy link
Member

@mxii-ca thanks!

@webknjaz webknjaz added the bug Something is broken label Jun 11, 2020
webknjaz added a commit that referenced this pull request Jul 13, 2020
@webknjaz
Mention PRs #291 and #298 in changelog
fd1c03a
@Safihre Safihre mentioned this pull request Jul 19, 2020
Closed
3 tasks
@mxii-ca mxii-ca deleted the bugfix/ssl_resource_warning branch July 21, 2020 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webknjaz webknjaz webknjaz approved these changes

Assignees
No one assigned
Labels
bug Something is broken
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

with "-X dev": ResourceWarning: unclosed <ssl.SSLSocket fd=4 ...
2 participants
@mxii-ca @webknjaz