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

Fix native HTTP server compatibility #1712

Merged
merged 7 commits into from Jun 16, 2018

Conversation

@webknjaz
Copy link
Member

@webknjaz webknjaz commented May 23, 2018

  • 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 #)
    #1377

  • What is the current behavior? (You can also link to an open issue here)
    Various exceptions related to str/bytes py2/3 compat are being raised.

  • What is the new behavior (if this is a feature change)?
    Will be working

  • Other information:
    Still WIP, don't merge.
    I hope to get some help from @jaraco.

  • 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
Copy link
Member

@jaraco jaraco left a comment

When I rebase your changes onto the bugfix/1377-add-tests branch, I notice that b936d6e fixes the failing test, suggesting that the second commit, 4c6b8ff, isn't necessary to fix the bug. Is there an additional bug that isn't caught by those tests, or can this second commit be omitted?

@@ -7,6 +7,7 @@
import cheroot.server

import cherrypy
from cherrypy._cpcompat import tonative

This comment has been minimized.

@jaraco

jaraco May 30, 2018
Member

I consider tonative to be deprecated. I'd like to avoid using it if possible and instead use either text or bytes explicitly on all Pythons.

@jaraco jaraco force-pushed the bugfix/1377-fix-native-http-server-compatibility branch from 4c6b8ff to 03d13a1 May 31, 2018
@codecov
Copy link

@codecov codecov bot commented May 31, 2018

Codecov Report

Merging #1712 into master will increase coverage by 0.45%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1712      +/-   ##
==========================================
+ Coverage   80.35%   80.81%   +0.45%     
==========================================
  Files         104      105       +1     
  Lines       13546    13563      +17     
==========================================
+ Hits        10885    10961      +76     
+ Misses       2661     2602      -59
@jaraco jaraco changed the title WIP: Fix native HTTP server compatibility Fix native HTTP server compatibility May 31, 2018
@cherrypy cherrypy deleted a comment May 31, 2018
@cherrypy cherrypy deleted a comment May 31, 2018
@jaraco
jaraco approved these changes May 31, 2018
Copy link
Member

@jaraco jaraco left a comment

I've stripped out the commit that seems to be unnecessary, including the one that fixes the reported issue. I believe this PR is ready to go.

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented May 31, 2018

@jaraco did it cause AppVeyor failures?

@@ -95,7 +95,7 @@ def send_response(self, status, headers, body):
req = self.req

# Set response status
req.status = str(status or '500 Server Error')
req.status = status or b'500 Server Error'

This comment has been minimized.

@webknjaz

webknjaz May 31, 2018
Author Member

@jaraco there's or '' over the place defaulting to bytes/unicode all over the place. Shall we try from __future__ import unicode_strings?

This comment has been minimized.

@jaraco

jaraco May 31, 2018
Member

I'd very much like to do that. It was a feature that wasn't available in the early implementations (Python 2.3 compatibility and all), and even some people still advise against its use. It's my opinion, however, that using from __future__ import unicode_literals is the best way to achieve compatibility. Let's do it as time permits.

This comment has been minimized.

@webknjaz

webknjaz May 31, 2018
Author Member

My concern is that from code it's not always obvious what kind of data the variable is expected to hold. Should we introduce some convention of prefixing b_ or suffixing _b to improve the situation?

@@ -0,0 +1,29 @@
"""Docstring."""

This comment has been minimized.

@webknjaz

webknjaz May 31, 2018
Author Member

@jaraco I guess we could explain what's going on in this test suite here.

@jaraco
Copy link
Member

@jaraco jaraco commented May 31, 2018

I do see appveyor failures; the failures seem unrelated, though I can't tell for sure. Is the startup/teardown of the native server causing conflicts with other tests?

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented May 31, 2018

I have no idea about the conflicts, but there's clearly some race conditions involved.

@cherrypy cherrypy deleted a comment May 31, 2018
@cherrypy cherrypy deleted a comment May 31, 2018
@cherrypy cherrypy deleted a comment May 31, 2018
@cherrypy cherrypy deleted a comment May 31, 2018
@cherrypy cherrypy deleted a comment May 31, 2018
@cherrypy cherrypy deleted a comment May 31, 2018
jaraco and others added 2 commits Jun 3, 2018
@cherrypy cherrypy deleted a comment Jun 4, 2018
@cherrypy cherrypy deleted a comment Jun 4, 2018
@cherrypy cherrypy deleted a comment Jun 4, 2018
@cherrypy cherrypy deleted a comment Jun 4, 2018
@cherrypy cherrypy deleted a comment Jun 4, 2018
@cherrypy cherrypy deleted a comment Jun 4, 2018
@jaraco jaraco merged commit ac39879 into master Jun 16, 2018
2 of 9 checks passed
2 of 9 checks passed
LGTM analysis: Python Running analyses for revisions
Details
ci/circleci: linux-build CircleCI is running your tests
Details
ci/circleci: macos-build Your tests are queued behind your running builds
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
WIP ready for review
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
@jaraco jaraco deleted the bugfix/1377-fix-native-http-server-compatibility branch Jun 16, 2018
@webknjaz webknjaz added this to Fixed in Bugs Jun 22, 2019
return sessions.BaseUrlSession(url)


def test_basic_request(cp_native_server):

This comment has been minimized.

@webknjaz

webknjaz Jun 22, 2019
Author Member

@jaraco it looks like this test doesn't catch the bug for some reason.

Ref:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bugs
Fixed
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants