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

write_any_response not sending optional causes failure on Samsung TV #73

Open
shaolo1 opened this issue Aug 20, 2018 · 6 comments
Open

Comments

@shaolo1
Copy link

shaolo1 commented Aug 20, 2018

I'm trying to work on an app that communicates with my Samsung TV. After some digging I figured out that the missing status text causes it to not work.

def write_any_response(response, write):
    if response.http_version != b"1.1":
        raise LocalProtocolError("I only send HTTP/1.1")
    status_bytes = str(response.status_code).encode("ascii")
    # We don't bother sending ascii status messages like "OK"; they're
    # optional and ignored by the protocol. (But the space after the numeric
    # status code is mandatory.)
    #
    # XX FIXME: could at least make an effort to pull out the status message
    # from stdlib's http.HTTPStatus table. Or maybe just steal their enums
    # (either by import or copy/paste). We already accept them as status codes
    # since they're of type IntEnum < int.
    write(bytesmod(b"HTTP/1.1 %s %s\r\n", (status_bytes, response.reason)))
write_headers(response.headers, write)

Seeing as how there is a FIXME here already. Would it be possible to get the following added?

from http import HTTPStatus
response.reason = HTTPStatus(response.status_code).name.encode('ascii')
@njsmith
Copy link
Member

njsmith commented Aug 20, 2018

That comment is out of date... As you can see from the code, h11 does send whatever "reason" text the user gives it.

Can you give more details on how you're using h11, and how you concluded that Samsung's http implementation is broken?

@shaolo1
Copy link
Author

shaolo1 commented Aug 20, 2018

I'm using Quart, which uses h11. The app I'm working on is not quite ready to be published. I've got it working with Flask, but the tv won't respond to the Quart version unless I populate the response.reason. By your comment, I'm assuming the response.reason should be populated before it gets to write_any_response().

I see asgi_send() being called with the message, but then it builds a new response using only the status.
self.send(h11.Response(status_code=self.response['status'], headers=headers))

Where is the correct place for me to supply the reason?

@njsmith
Copy link
Member

njsmith commented Aug 20, 2018

The quick fix would be to pass the reason phrase into the h11.Response constructor, like: h11.Response(status_code=self.response['status'], headers=headers, reason="blah blah")

Out of curiosity, does your TV require a specific reason phrase, or does it just insist that it be non-empty?

Given that this is breaking things (wtf is wrong with you, samsung TV), maybe we should also start defaulting the reason inside h11 when it's not specified. Probably the way to do this would be in the Reponse/InformationalResponse constructor, if the reason argument isn't passed, set a default based on the status code.

@shaolo1
Copy link
Author

shaolo1 commented Aug 20, 2018

Interestingly enough its completely happy with b'HTTP/1.1 200 foo\r\n' or b'HTTP/1.1 206 foo\r\n'

@njsmith
Copy link
Member

njsmith commented Aug 21, 2018

Heh. Well, I guess that's another option: if reason isn't given, default to foo.

(I'm joking!)

@shaolo1
Copy link
Author

shaolo1 commented Aug 22, 2018

I finally got around to posting the app I mentioned that has the problem on my tv. https://github.com/shaolo1/VideoServer. I've temporarily monkey patched the h11 issue to get it working.

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

2 participants