This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2794

hgweb: refactor 304 handling code
ClosedPublic

Authored by indygreg on Mar 11 2018, 12:24 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGccb70a77f746: hgweb: refactor 304 handling code
Summary

We had generic code in wsgirequest for handling HTTP 304 responses.
We also had a special case for it in the catch all exception handler
in the WSGI application.

We only ever raise 304 in one place. So, we don't need to treat it
specially in the catch all exception handler.

But it is useful to validate behavior of 304 responses. We port the
code that sends a 304 to use the new response API. We then move the
code for screening 304 sanity into the new response API.

As part of doing so, we discovered that we would send
Content-Length: 0. This is not allowed. So, we fix our response code
to not emit that header for empty response bodies.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Mar 11 2018, 12:24 AM
durin42 accepted this revision.Mar 12 2018, 5:14 PM
This revision is now accepted and ready to land.Mar 12 2018, 5:14 PM
Closed by commit rHGccb70a77f746: hgweb: refactor 304 handling code (authored by indygreg). · Explain WhyMar 12 2018, 5:33 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 6930

mercurial/hgweb/hgweb_mod.py

import contextlib import contextlib
import os import os
from .common import ( from .common import (
ErrorResponse, ErrorResponse,
HTTP_BAD_REQUEST, HTTP_BAD_REQUEST,
HTTP_NOT_FOUND, HTTP_NOT_FOUND,
HTTP_NOT_MODIFIED,
HTTP_OK, HTTP_OK,
HTTP_SERVER_ERROR, HTTP_SERVER_ERROR,
cspvalues, cspvalues,
permhooks, permhooks,
) )
from .. import ( from .. import (
encoding, encoding,
req.qsparams['cmd'] = tmpl.cache['default'] req.qsparams['cmd'] = tmpl.cache['default']
cmd = req.qsparams['cmd'] cmd = req.qsparams['cmd']
# Don't enable caching if using a CSP nonce because then it wouldn't # Don't enable caching if using a CSP nonce because then it wouldn't
# be a nonce. # be a nonce.
if rctx.configbool('web', 'cache') and not rctx.nonce: if rctx.configbool('web', 'cache') and not rctx.nonce:
tag = 'W/"%d"' % self.mtime tag = 'W/"%d"' % self.mtime
if req.headers.get('If-None-Match') == tag: if req.headers.get('If-None-Match') == tag:
raise ErrorResponse(HTTP_NOT_MODIFIED) res.status = '304 Not Modified'
# Response body not allowed on 304.
res.setbodybytes('')
return res.sendresponse()
wsgireq.headers.append((r'ETag', pycompat.sysstr(tag))) wsgireq.headers.append((r'ETag', pycompat.sysstr(tag)))
res.headers['ETag'] = tag res.headers['ETag'] = tag
if cmd not in webcommands.__all__: if cmd not in webcommands.__all__:
msg = 'no such method: %s' % cmd msg = 'no such method: %s' % cmd
raise ErrorResponse(HTTP_BAD_REQUEST, msg) raise ErrorResponse(HTTP_BAD_REQUEST, msg)
else: else:
not isinstance(err, error.ManifestLookupError)): not isinstance(err, error.ManifestLookupError)):
msg = 'revision not found: %s' % err.name msg = 'revision not found: %s' % err.name
return tmpl('error', error=msg) return tmpl('error', error=msg)
except (error.RepoError, error.RevlogError) as inst: except (error.RepoError, error.RevlogError) as inst:
wsgireq.respond(HTTP_SERVER_ERROR, ctype) wsgireq.respond(HTTP_SERVER_ERROR, ctype)
return tmpl('error', error=pycompat.bytestr(inst)) return tmpl('error', error=pycompat.bytestr(inst))
except ErrorResponse as inst: except ErrorResponse as inst:
wsgireq.respond(inst, ctype) wsgireq.respond(inst, ctype)
if inst.code == HTTP_NOT_MODIFIED:
# Not allowed to return a body on a 304
return ['']
return tmpl('error', error=pycompat.bytestr(inst)) return tmpl('error', error=pycompat.bytestr(inst))
def check_perm(self, rctx, req, op): def check_perm(self, rctx, req, op):
for permhook in permhooks: for permhook in permhooks:
permhook(rctx, req, op) permhook(rctx, req, op)
def getwebview(repo): def getwebview(repo):
"""The 'web.view' config controls changeset filter to hgweb. Possible """The 'web.view' config controls changeset filter to hgweb. Possible

mercurial/hgweb/request.py

import errno import errno
import socket import socket
import wsgiref.headers as wsgiheaders import wsgiref.headers as wsgiheaders
#import wsgiref.validate #import wsgiref.validate
from .common import ( from .common import (
ErrorResponse, ErrorResponse,
HTTP_NOT_MODIFIED,
statusmessage, statusmessage,
) )
from ..thirdparty import ( from ..thirdparty import (
attr, attr,
) )
from .. import ( from .. import (
error, error,
self._bodywritefn = None self._bodywritefn = None
def _verifybody(self): def _verifybody(self):
if (self._bodybytes is not None or self._bodygen is not None if (self._bodybytes is not None or self._bodygen is not None
or self._bodywillwrite): or self._bodywillwrite):
raise error.ProgrammingError('cannot define body multiple times') raise error.ProgrammingError('cannot define body multiple times')
def setbodybytes(self, b): def setbodybytes(self, b):
"""Define the response body as static bytes.""" """Define the response body as static bytes.
The empty string signals that there is no response body.
"""
self._verifybody() self._verifybody()
self._bodybytes = b self._bodybytes = b
self.headers['Content-Length'] = '%d' % len(b) self.headers['Content-Length'] = '%d' % len(b)
def setbodygen(self, gen): def setbodygen(self, gen):
"""Define the response body as a generator of bytes.""" """Define the response body as a generator of bytes."""
self._verifybody() self._verifybody()
self._bodygen = gen self._bodygen = gen
if not self.status: if not self.status:
raise error.ProgrammingError('status line not defined') raise error.ProgrammingError('status line not defined')
if (self._bodybytes is None and self._bodygen is None if (self._bodybytes is None and self._bodygen is None
and not self._bodywillwrite): and not self._bodywillwrite):
raise error.ProgrammingError('response body not defined') raise error.ProgrammingError('response body not defined')
# RFC 7232 Section 4.1 states that a 304 MUST generate one of
# {Cache-Control, Content-Location, Date, ETag, Expires, Vary}
# and SHOULD NOT generate other headers unless they could be used
# to guide cache updates. Furthermore, RFC 7230 Section 3.3.2
# states that no response body can be issued. Content-Length can
# be sent. But if it is present, it should be the size of the response
# that wasn't transferred.
if self.status.startswith('304 '):
# setbodybytes('') will set C-L to 0. This doesn't conform with the
# spec. So remove it.
if self.headers.get('Content-Length') == '0':
del self.headers['Content-Length']
# Strictly speaking, this is too strict. But until it causes
# problems, let's be strict.
badheaders = {k for k in self.headers.keys()
if k.lower() not in ('date', 'etag', 'expires',
'cache-control',
'content-location',
'vary')}
if badheaders:
raise error.ProgrammingError(
'illegal header on 304 response: %s' %
', '.join(sorted(badheaders)))
if self._bodygen is not None or self._bodywillwrite:
raise error.ProgrammingError("must use setbodybytes('') with "
"304 responses")
# Various HTTP clients (notably httplib) won't read the HTTP response # Various HTTP clients (notably httplib) won't read the HTTP response
# until the HTTP request has been sent in full. If servers (us) send a # until the HTTP request has been sent in full. If servers (us) send a
# response before the HTTP request has been fully sent, the connection # response before the HTTP request has been fully sent, the connection
# may deadlock because neither end is reading. # may deadlock because neither end is reading.
# #
# We work around this by "draining" the request data before # We work around this by "draining" the request data before
# sending any response in some conditions. # sending any response in some conditions.
drain = False drain = False
self.headers.append((r'Content-Length', str(len(body)))) self.headers.append((r'Content-Length', str(len(body))))
for k, v in self.headers: for k, v in self.headers:
if not isinstance(v, str): if not isinstance(v, str):
raise TypeError('header value must be string: %r' % (v,)) raise TypeError('header value must be string: %r' % (v,))
if isinstance(status, ErrorResponse): if isinstance(status, ErrorResponse):
self.headers.extend(status.headers) self.headers.extend(status.headers)
if status.code == HTTP_NOT_MODIFIED:
# RFC 2616 Section 10.3.5: 304 Not Modified has cases where
# it MUST NOT include any headers other than these and no
# body
self.headers = [(k, v) for (k, v) in self.headers if
k in ('Date', 'ETag', 'Expires',
'Cache-Control', 'Vary')]
status = statusmessage(status.code, pycompat.bytestr(status)) status = statusmessage(status.code, pycompat.bytestr(status))
elif status == 200: elif status == 200:
status = '200 Script output follows' status = '200 Script output follows'
elif isinstance(status, int): elif isinstance(status, int):
status = statusmessage(status) status = statusmessage(status)
# Various HTTP clients (notably httplib) won't read the HTTP # Various HTTP clients (notably httplib) won't read the HTTP
# response until the HTTP request has been sent in full. If servers # response until the HTTP request has been sent in full. If servers