cheroot icon indicating copy to clipboard operation
cheroot copied to clipboard

Handle `OpenSSL.SSL.WantReadError` and `WantWriteError` in pyopenssl adapter

Open vashek opened this issue 4 years ago β€’ 9 comments

❓ What kind of change does this PR introduce?

  • [x] 🐞 bug fix
  • [ ] 🐣 feature
  • [ ] πŸ“‹ docs update
  • [ ] πŸ“‹ tests/coverage improvement
  • [ ] πŸ“‹ refactoring
  • [ ] πŸ’₯ other

πŸ“‹ What is the related issue number (starting with #)

Fixes #245

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

❓ 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

vashek avatar Nov 02 '20 12:11 vashek

@vashek have you considered fixing pyopenssl itself? https://github.com/pyca/pyopenssl/issues/176#issuecomment-720471766

webknjaz avatar Nov 02 '20 13:11 webknjaz

Hm... it looks like sendall was used as write() + flush() at some point but then got replaced with write: https://github.com/cherrypy/cheroot/commit/60cd709551d564551d91a5b170c311bb23ad2ac6 / https://github.com/cherrypy/cheroot/commit/a0b1d018c912aa21473bac3f99d46018faed5563.

webknjaz avatar Nov 02 '20 16:11 webknjaz

Some jobs are failing because of a deprecation warning in cryptography. I've fixed that on master. To get the changes, rebase this PR branch.

webknjaz avatar Nov 02 '20 16:11 webknjaz

Do you think this can be tested somehow?

webknjaz avatar Nov 04 '20 15:11 webknjaz

@vashek so what do you think about tests?

webknjaz avatar Nov 04 '20 23:11 webknjaz

The test will probably need to make the sending and/or receiving socket full like I tried here: https://github.com/pyca/pyopenssl/pull/955/files#diff-5fbedfbbf8f0780aeee680927973f302330dc10fe915c7e7deecf4b0556c492cR3157

webknjaz avatar Nov 23 '20 01:11 webknjaz

Sorry for letting this rot. Quite honestly, I'd appreciate if someone could help with the tests - or accept it without them.

vashek avatar Apr 15 '21 00:04 vashek

Any chance of accepting this?

vashek avatar Feb 08 '22 23:02 vashek

@vashek I suppose it's hard to write tests for this which is why I'll probably accept it without tests. But https://github.com/cherrypy/cheroot/issues/245#issuecomment-1465341867 seems to imply that it may be causing problems for retries. Do you have any thoughts on that?

webknjaz avatar Mar 17 '23 02:03 webknjaz