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

Address #358: Only process valid connections #359

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

cameronbrunner
Copy link
Contributor

@cameronbrunner cameronbrunner commented Jan 15, 2021

The _from_server_socket routine may return a None which should not be sent to workers to process.

❓ 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 #)

#358

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

#358

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

πŸ“‹ Other information:

πŸ“‹ 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
Copy link
Member

@liamstask what's your opinion on this?

@webknjaz
Copy link
Member

@cameronbrunner this looks like a simple change but, as you've demonstrated, missing tiny details may lead to disastrous server states. So could you please add a pytest-based regression test verifying this behavior?

@liamstask
Copy link
Contributor

Seems reasonable to me as an immediate fix for this issue, though agreed we need a regression test.

Would also be good to add a docstring/comment to _from_server_socket() documenting this behavior.

This behavior may also explain the unresponsive behavior, as a result of SSL errors, reported in #346

For separate follow up:

  • thread pool should also likely use _SHUTDOWNREQUEST = object() to create a dedicated sentinel rather than reusing None
  • _from_server_socket() is generally ripe for improvement and refactoring

The `_from_server_socket routine` may return a None which should
not be sent to workers to process.

for cherrypy#358
@webknjaz webknjaz added the bug Something is broken label Jan 18, 2021
@webknjaz webknjaz merged commit ff630b1 into cherrypy:master Jan 18, 2021
@webknjaz
Copy link
Member

I'm merging this as a hotfix and I hope improvements will get added over time too.

@webknjaz
Copy link
Member

Released as v8.5.2 @ https://pypi.org/project/cheroot/8.5.2/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants