From f010e3a01b841e30e901b04af6449d1b87b1399e Mon Sep 17 00:00:00 2001 From: eikek Date: Sat, 2 Mar 2024 22:10:14 +0100 Subject: [PATCH] Fix range responses Use a fixed chunk-size in case the request specifies an open range which would mean to transport the entire file. In this case, the client gets some smaller chunk as it is already indicating to understand range responses. If a file is smaller than this chunksize, it will be a normal response. Fixes: #1328 --- .../sharry/backend/config/FilesConfig.scala | 4 +- .../src/main/resources/reference.conf | 4 + .../restserver/routes/ByteResponse.scala | 99 +++++++++++-------- .../restserver/routes/OpenShareRoutes.scala | 8 +- .../restserver/routes/ShareRoutes.scala | 24 ++++- 5 files changed, 95 insertions(+), 44 deletions(-) diff --git a/modules/backend/src/main/scala/sharry/backend/config/FilesConfig.scala b/modules/backend/src/main/scala/sharry/backend/config/FilesConfig.scala index 6d8ffd9f..43860b0f 100644 --- a/modules/backend/src/main/scala/sharry/backend/config/FilesConfig.scala +++ b/modules/backend/src/main/scala/sharry/backend/config/FilesConfig.scala @@ -3,13 +3,15 @@ package sharry.backend.config import cats.data.{Validated, ValidatedNec} import cats.syntax.all._ +import sharry.common.ByteSize import sharry.common.Ident import sharry.store.FileStoreConfig case class FilesConfig( defaultStore: Ident, stores: Map[Ident, FileStoreConfig], - copyFiles: CopyFilesConfig + copyFiles: CopyFilesConfig, + downloadChunkSize: ByteSize ) { val enabledStores: Map[Ident, FileStoreConfig] = diff --git a/modules/restserver/src/main/resources/reference.conf b/modules/restserver/src/main/resources/reference.conf index 5db5e4f2..740fc2b8 100644 --- a/modules/restserver/src/main/resources/reference.conf +++ b/modules/restserver/src/main/resources/reference.conf @@ -358,6 +358,10 @@ sharry.restserver { # How many files to copy in parallel. parallel = 2 } + + # For open range requests, use this amount of data when + # responding. + download-chunk-size = "4M" } # Checksums of uploaded files are computed in the background. diff --git a/modules/restserver/src/main/scala/sharry/restserver/routes/ByteResponse.scala b/modules/restserver/src/main/scala/sharry/restserver/routes/ByteResponse.scala index 39961bf4..6fbd9ab5 100644 --- a/modules/restserver/src/main/scala/sharry/restserver/routes/ByteResponse.scala +++ b/modules/restserver/src/main/scala/sharry/restserver/routes/ByteResponse.scala @@ -3,6 +3,7 @@ package sharry.restserver.routes import cats.data.OptionT import cats.effect.Sync import cats.implicits._ +import fs2.Stream import sharry.backend.BackendApp import sharry.backend.share._ @@ -23,36 +24,34 @@ object ByteResponse { backend: BackendApp[F], shareId: ShareId, pass: Option[Password], + chunkSize: ByteSize, fid: Ident - ) = + ): F[Response[F]] = req.headers .get[Range] .map(_.ranges.head) - .map(sr => range(dsl, sr, backend, shareId, pass, fid)) + .map(sr => range(dsl, req, sr, backend, shareId, pass, chunkSize, fid)) .getOrElse(all(dsl, req, backend, shareId, pass, fid)) def range[F[_]: Sync]( dsl: Http4sDsl[F], + req: Request[F], sr: Range.SubRange, backend: BackendApp[F], shareId: ShareId, pass: Option[Password], + chunkSize: ByteSize, fid: Ident ): F[Response[F]] = { import dsl._ - val rangeDef: ByteRange = sr.second - .map(until => ByteRange(sr.first, (until - sr.first + 1).toInt)) - .getOrElse { - if (sr.first == 0) ByteRange.All - else ByteRange(sr.first, Int.MaxValue) - } - + val rangeDef = makeBinnyByteRange(sr, chunkSize) (for { file <- backend.share.loadFile(shareId, fid, pass, rangeDef) resp <- OptionT.liftF { if (rangeInvalid(file.fileMeta, sr)) RangeNotSatisfiable() - else partialResponse(dsl, file, sr) + else if (file.fileMeta.length <= chunkSize) allBytes(dsl, req, file) + else partialResponse(dsl, req, file, chunkSize, sr) } } yield resp).getOrElseF(NotFound()) } @@ -69,27 +68,34 @@ object ByteResponse { (for { file <- backend.share.loadFile(shareId, fid, pass, ByteRange.All) - resp <- OptionT.liftF( - etagResponse(dsl, req, file).getOrElseF( - Ok.apply(file.data) - .map(setETag(file.fileMeta)) - .map( - _.putHeaders( - `Content-Type`(mediaType(file)), - `Accept-Ranges`.bytes, - `Last-Modified`(timestamp(file)), - `Content-Disposition`("inline", fileNameMap(file)), - fileSizeHeader(file.fileMeta.length) - ) - ) - ) - ) + resp <- OptionT.liftF(allBytes(dsl, req, file)) } yield resp).getOrElseF(NotFound()) } - private def setETag[F[_]](fm: RFileMeta)(r: Response[F]): Response[F] = - if (fm.checksum.isEmpty) r - else r.putHeaders(ETag(fm.checksum.toHex)) + def allBytes[F[_]: Sync]( + dsl: Http4sDsl[F], + req: Request[F], + file: FileRange[F] + ): F[Response[F]] = { + import dsl._ + + val isHead = req.method == Method.HEAD + val data = if (!isHead) file.data else Stream.empty + etagResponse(dsl, req, file).getOrElseF( + Ok(data) + .map(setETag(file.fileMeta)) + .map( + _.putHeaders( + `Content-Type`(mediaType(file)), + `Accept-Ranges`.bytes, + `Last-Modified`(timestamp(file)), + `Content-Disposition`("inline", fileNameMap(file)), + `Content-Length`(file.fileMeta.length.bytes), + fileSizeHeader(file.fileMeta.length) + ) + ) + ) + } private def etagResponse[F[_]: Sync]( dsl: Http4sDsl[F], @@ -106,31 +112,44 @@ object ByteResponse { private def partialResponse[F[_]: Sync]( dsl: Http4sDsl[F], + req: Request[F], file: FileRange[F], + chunkSize: ByteSize, range: Range.SubRange ): F[Response[F]] = { import dsl._ - val len = file.fileMeta.length - val respLen = range.second.getOrElse(len.bytes) - range.first + 1 - PartialContent(file.data.take(respLen)).map( + + val fileLen = file.fileMeta.length + val respLen = + range.second.map(until => until - range.first + 1).getOrElse(chunkSize.bytes) + val respRange = + Range.SubRange(range.first, range.second.getOrElse(range.first + chunkSize.bytes)) + + val isHead = req.method == Method.HEAD + val data = if (isHead) Stream.empty else file.data.take(respLen.toLong) + PartialContent(data).map( _.withHeaders( `Accept-Ranges`.bytes, `Content-Type`(mediaType(file)), `Last-Modified`(timestamp(file)), `Content-Disposition`("inline", fileNameMap(file)), - fileSizeHeader(ByteSize(respLen)), - `Content-Range`(RangeUnit.Bytes, subRangeResp(range, len.bytes), Some(len.bytes)) + fileSizeHeader(file.fileMeta.length), + `Content-Range`(RangeUnit.Bytes, respRange, Some(fileLen.bytes)) ) ) } - private def subRangeResp(in: Range.SubRange, length: Long): Range.SubRange = - in match { - case Range.SubRange(n, None) => - Range.SubRange(n, Some(length)) - case Range.SubRange(n, Some(t)) => - Range.SubRange(n, Some(t)) - } + private def makeBinnyByteRange(sr: Range.SubRange, chunkSize: ByteSize): ByteRange = + sr.second + .map(until => ByteRange(sr.first, (until - sr.first + 1).toInt)) + .getOrElse { + if (sr.first == 0) ByteRange(0, chunkSize.bytes.toInt) + else ByteRange(sr.first, chunkSize.bytes.toInt) + } + + private def setETag[F[_]](fm: RFileMeta)(r: Response[F]): Response[F] = + if (fm.checksum.isEmpty) r + else r.putHeaders(ETag(fm.checksum.toHex)) private def rangeInvalid(file: RFileMeta, range: Range.SubRange): Boolean = range.first < 0 || range.second.exists(t => t < range.first || t > file.length.bytes) diff --git a/modules/restserver/src/main/scala/sharry/restserver/routes/OpenShareRoutes.scala b/modules/restserver/src/main/scala/sharry/restserver/routes/OpenShareRoutes.scala index f7e6d578..0cc8f788 100644 --- a/modules/restserver/src/main/scala/sharry/restserver/routes/OpenShareRoutes.scala +++ b/modules/restserver/src/main/scala/sharry/restserver/routes/OpenShareRoutes.scala @@ -24,9 +24,13 @@ object OpenShareRoutes { case req @ GET -> Root / Ident(id) / "file" / Ident(fid) => val pw = SharryPassword(req) - ByteResponse(dsl, req, backend, ShareId.publish(id), pw, fid) + val chunkSize = cfg.backend.files.downloadChunkSize + ByteResponse(dsl, req, backend, ShareId.publish(id), pw, chunkSize, fid) + case req @ HEAD -> Root / Ident(id) / "file" / Ident(fid) => + val pw = SharryPassword(req) + val chunkSize = cfg.backend.files.downloadChunkSize + ByteResponse(dsl, req, backend, ShareId.publish(id), pw, chunkSize, fid) } } - } diff --git a/modules/restserver/src/main/scala/sharry/restserver/routes/ShareRoutes.scala b/modules/restserver/src/main/scala/sharry/restserver/routes/ShareRoutes.scala index 8ba76f12..4c7bea8a 100644 --- a/modules/restserver/src/main/scala/sharry/restserver/routes/ShareRoutes.scala +++ b/modules/restserver/src/main/scala/sharry/restserver/routes/ShareRoutes.scala @@ -71,7 +71,29 @@ object ShareRoutes { case req @ GET -> Root / Ident(id) / "file" / Ident(fid) => val pw = SharryPassword(req) - ByteResponse(dsl, req, backend, ShareId.secured(id, token.account), pw, fid) + val chunkSize = cfg.backend.files.downloadChunkSize + ByteResponse( + dsl, + req, + backend, + ShareId.secured(id, token.account), + pw, + chunkSize, + fid + ) + + case req @ HEAD -> Root / Ident(id) / "file" / Ident(fid) => + val pw = SharryPassword(req) + val chunkSize = cfg.backend.files.downloadChunkSize + ByteResponse( + dsl, + req, + backend, + ShareId.secured(id, token.account), + pw, + chunkSize, + fid + ) // make it safer by also using the share id case DELETE -> Root / Ident(_) / "file" / Ident(fid) =>