launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/services/librarianserver/web.py'
> --- lib/lp/services/librarianserver/web.py	2018-01-01 17:40:03 +0000
> +++ lib/lp/services/librarianserver/web.py	2018-04-06 08:39:18 +0000
> @@ -188,76 +194,109 @@
>          return defaultResource.render(request)
>  
>  
> -class File(static.File):
> +class File(resource.Resource):
>      isLeaf = True
>  
> -    def __init__(
> -        self, contentType, encoding, modification_time, stream, size):
> +    def __init__(self, contentType, encoding, modification_time, stream, size):
> +        resource.Resource.__init__(self)
>          # Have to convert the UTC datetime to POSIX timestamp (localtime)
>          offset = datetime.utcnow() - datetime.now()
>          local_modification_time = modification_time - offset
>          self._modification_time = time.mktime(
>              local_modification_time.timetuple())
> -        static.File.__init__(self, '.')
>          self.type = contentType
>          self.encoding = encoding
>          self.stream = stream
>          self.size = size
>  
> -    def getModificationTime(self):
> -        """Override the time on disk with the time from the database.
> -
> -        This is used by twisted to set the Last-Modified: header.
> -        """
> -        return self._modification_time
> -
> -    def restat(self, reraise=True):
> -        return  # Noop
> -
> -    def getsize(self):
> -        return self.size
> -
> -    def exists(self):
> -        return self.stream is not None
> -
> -    def isdir(self):
> -        return False
> -
> -    def openForReading(self):
> -        return self.stream
> -
> -    def makeProducer(self, request, fileForReading):
> -        # Unfortunately, by overriding the static.File's more
> -        # complex makeProducer method we lose HTTP range support.
> -        # However, this seems the only sane way of coping with the fact
> -        # that sucking data in from Swift requires a Deferred and the
> -        # static.*Producer implementations don't cope. This shouldn't be
> -        # a problem as the Librarian sits behind Squid. If it is, I
> -        # think we will need to cargo-cult three Procucer
> -        # implementations in static, making the small modification to
> -        # cope with self.fileObject.read maybe returning a Deferred, and
> +    def _setContentHeaders(self, request):
> +        request.setHeader(b'content-length', intToBytes(self.size))
> +        if self.type:
> +            request.setHeader(b'content-type', networkString(self.type))
> +        if self.encoding:
> +            request.setHeader(
> +                b'content-encoding', networkString(self.encoding))
> +
> +    def render_GET(self, request):
> +        """See `Resource`."""
> +        request.setHeader(b'accept-ranges', b'none')
> +
> +        if request.setLastModified(self._modification_time) is http.CACHED:
> +            # `setLastModified` also sets the response code for us, so if
> +            # the request is cached, we close the file now that we've made
> +            # sure that the request would otherwise succeed and return an
> +            # empty body.
> +            self.stream.close()
> +            return b''
> +
> +        if request.method == b'HEAD':
> +            # Set the content headers here, rather than making a producer.
> +            self._setContentHeaders(request)
> +            self.stream.close()
> +            return b''
> +
> +        # static.File has HTTP range support, which would be nice to have.
> +        # Unfortunately, static.File isn't a good match for producing data
> +        # dynamically by fetching it from Swift. This shouldn't be a problem
> +        # as the Librarian sits behind Squid. If it is, I think we will need

Well actually...

> +        # to cargo-cult the byte-range support and three Producer
> +        # implementations from static.File, making the small modifications
> +        # to cope with self.fileObject.read maybe returning a Deferred, and
>          # the static.File.makeProducer method to return the correct
>          # producer.
>          self._setContentHeaders(request)
>          request.setResponseCode(http.OK)
> -        return FileProducer(request, fileForReading)
> -
> -
> -class FileProducer(static.NoRangeStaticProducer):
> +        producer = FileProducer(request, self.stream)
> +        producer.start()
> +
> +        return server.NOT_DONE_YET
> +
> +
> +@implementer(IPushProducer)
> +class FileProducer(object):
> +
> +    buffer_size = abstract.FileDescriptor.bufferSize
> +
> +    def __init__(self, request, stream):
> +        self.request = request
> +        self.stream = stream
> +        self.producing = True
> +
> +    def start(self):
> +        self.request.registerProducer(self, True)
> +        self.resumeProducing()
> +
> +    def pauseProducing(self):
> +        """See `IPushProducer`."""
> +        self.producing = False
> +
>      @defer.inlineCallbacks
> +    def _produceFromStream(self):
> +        """Read data from our stream and write it to our consumer."""
> +        while self.request and self.producing:
> +            data = yield self.stream.read(self.buffer_size)
> +            # pauseProducing or stopProducing may have been called while we
> +            # were waiting.
> +            if not self.producing:
> +                return
> +            if data:
> +                self.request.write(data)
> +            else:
> +                self.request.unregisterProducer()
> +                self.request.finish()
> +                self.stopProducing()
> +
>      def resumeProducing(self):
> -        if not self.request:
> -            return
> -        data = yield self.fileObject.read(self.bufferSize)
> -        # stopProducing may have been called while we were waiting.
> -        if not self.request:
> -            return
> -        if data:
> -            self.request.write(data)
> -        else:
> -            self.request.unregisterProducer()
> -            self.request.finish()
> -            self.stopProducing()
> +        """See `IPushProducer`."""
> +        self.producing = True
> +        if self.request:
> +            reactor.callLater(0, self._produceFromStream)
> +
> +    def stopProducing(self):
> +        """See `IProducer`."""
> +        self.producing = False
> +        self.stream.close()
> +        self.request = None
>  
>  
>  class DigestSearchResource(resource.Resource):


-- 
https://code.launchpad.net/~cjwatson/launchpad/twisted-17.9.0/+merge/342777
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.