Skip to content

Current cherrypy 18.0.0 fails to open websocket connection with ws4py 0.5.1 #1738

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

Closed
2 tasks
ralhei opened this issue Sep 6, 2018 · 17 comments
Closed
2 tasks

Comments

@ralhei
Copy link

ralhei commented Sep 6, 2018

  • I'm submitting a ...
  • [x ] bug report
  • feature request
  • question about the decisions made in the repository
  • What is the current behavior?
    The browser (firefox) fails to make a connection to the /ws endpoint. Downgrading to cherrypy 17.3.0 or earlier doesn't show this behavior, everything works fine there.

  • If the current behavior is a bug, please provide the steps to reproduce and if possible a screenshots and logs of the problem. If you can, show us your code.

  • Please tell us about your environment:

  • Cheroot version: 6.5.2
  • CherryPy version: 18.0.0
  • Python version: 3.6.4
  • OS: centos7
  • Browser: Firefox
  • Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, e.g. stackoverflow, gitter, etc.)
@webknjaz
Copy link
Member

webknjaz commented Sep 8, 2018

More details needed. Currently there's no reproducer.
cc @Lawouach

@Lawouach
Copy link
Contributor

Lawouach commented Sep 8, 2018

Hey there,

I will give it a look but the ws4py is considered a project that has not been worked on for a few years now so I'm not surprised considering it was tapping into the internals of CherryPy.

@Lawouach
Copy link
Contributor

Lawouach commented Sep 8, 2018

Okay, so the issue is that CherryPy response headers seems to now be unicode when ws4py was passing byte objects.

Could someone confirm me this is the case with cheroot and when this change happened? So I can add the right fix in place in ws4py.

@webknjaz
Copy link
Member

webknjaz commented Sep 8, 2018

@Lawouach I bet it's CherryPy switching to Python 3.5+ starting v18.0.0, but might be wrong.

cc @jaraco

@jaraco
Copy link
Member

jaraco commented Sep 8, 2018

Hmm. If I recall correctly, the change for v18.0.0 was mainly in metadata. Here are the differences.

@jaraco
Copy link
Member

jaraco commented Sep 8, 2018

I suspect the issue is with d3fa6b8, as text_or_bytes is different from string_types on Python 3.

@webknjaz
Copy link
Member

webknjaz commented Sep 8, 2018

Yeah, that's also my guess here.

@jaraco
Copy link
Member

jaraco commented Sep 8, 2018

Question is - do we wish in CherryPy to restore support for passing headers as bytes? I think we'd like to get away from supporting both bytes/text in a variety of interfaces. If we do wish to support this use case, we should write a unit test to capture that expectation. But my instinct is we should ask downstream consumers to supply text headers.

@webknjaz
Copy link
Member

webknjaz commented Sep 8, 2018

Oh, wait. I've merged this recently: #1736

@webknjaz
Copy link
Member

webknjaz commented Sep 8, 2018

@ralhei @Lawouach can anyone verify whether it's reproducible on master?

@Lawouach
Copy link
Contributor

Lawouach commented Sep 8, 2018

Just tried. Couldn't see the same problem with master indeed.

@webknjaz
Copy link
Member

webknjaz commented Sep 8, 2018

So we just need to cut a new patch version then :)

@Lawouach
Copy link
Contributor

Lawouach commented Sep 8, 2018

I think that would do :p

@jaraco
Copy link
Member

jaraco commented Sep 8, 2018

Releasing as v18.0.1.

@jaraco jaraco closed this as completed Sep 8, 2018
@Lawouach
Copy link
Contributor

Lawouach commented Sep 9, 2018

Thanks folks for being so responsive and swift!

@ralhei
Copy link
Author

ralhei commented Sep 10, 2018

Hi everybody, sorry for being so unresponsive, but I've been away all weekend.You guys did a great job, ws4py is now working again! Thanks so much for this super fast patch.

@webknjaz
Copy link
Member

You should thank to @tobiashenkel, the actual submitter of the patch :)

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

No branches or pull requests

4 participants