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

Use HTTP/1.1 #617

Merged
merged 1 commit into from
Oct 4, 2017
Merged

Use HTTP/1.1 #617

merged 1 commit into from
Oct 4, 2017

Conversation

jart
Copy link
Contributor

@jart jart commented Oct 4, 2017

  • Order of magnitude reduction in TCP sockets and Python threads
  • Small improvement on overall page loading time

@jart jart requested a review from nfelt October 4, 2017 20:42
@@ -261,6 +261,12 @@ def main(unused_argv=None):
efi.inspect(FLAGS.logdir, event_file, FLAGS.tag)
return 0
else:

# If we don't use HTTP/1.1 then a new TCP socket and Python thread is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe clarify that HTTP/1.0 is the default? Esp. since HTTP/2 does exist now.

Also, just checking but I assume we do in fact always sent a Content-Length header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and yes, everything sets content-length, assuming it's using http_util.Respond. We actually used to use HTTP/1.1 before we accidentally stopped doing it when we added Werkzeug. That helper was designed specifically to make sure important headers like that got set.

- Order of magnitude reduction in TCP sockets and Python threads
- Small improvement on overall page loading time
@jart
Copy link
Contributor Author

jart commented Oct 4, 2017

PTAL

@@ -261,6 +261,13 @@ def main(unused_argv=None):
efi.inspect(FLAGS.logdir, event_file, FLAGS.tag)
return 0
else:

# The default is HTTP/1.0 for some strange reason. If we don't use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, the BasicHTTPRequestHandler docs say it's to keep people from shooting themselves in the foot by omitting Content-Length:
https://docs.python.org/2/library/basehttpserver.html#BaseHTTPServer.BaseHTTPRequestHandler.protocol_version

But yeah, it seems like HTTP/1.1 should be the default and BaseHTTPRequestHandler should just die on you if you attempt to send a response with no content length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

@jart jart merged commit ef83e6c into tensorflow:master Oct 4, 2017
@jart jart deleted the http11 branch October 4, 2017 22:11
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

Successfully merging this pull request may close these issues.

2 participants