Skip to content

Prevent 503's on attachment uploads #709

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prevent 503's on attachment uploads #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions modules/service/src/main/scala/lucuma/odb/graphql/ObsAttachmentRoutes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package lucuma.odb.graphql

import cats.effect._
import cats.effect.implicits._
import cats.effect.std.UUIDGen
import cats.implicits._
import eu.timepit.refined.types.string.NonEmptyString
Expand Down Expand Up @@ -97,6 +98,7 @@ object ObsAttachmentRoutes {
s
.insertAttachment(user, programId, typeTag, fileName, description, req.body)
.flatMap(id => Ok(id.toString))
.guarantee(req.body.compile.drain)
Copy link
Contributor Author

@toddburnside toddburnside Sep 12, 2023

Choose a reason for hiding this comment

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

What are the unforeseen consequences of always draining the stream regardless of whether it has already been fully or partially read? In some local tests with a 3+M file that was successfully uploaded, a compile.toList where the compile.drain is was reading values of <64K bytes.

Copy link
Contributor

@armanbilge armanbilge Sep 12, 2023

Choose a reason for hiding this comment

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

Ugh this is a really good question. I'm not sure it is well-specified, it may be implementation specific, and I have to take a look.

Are we currently using Blaze server? Do you think we can switch to Ember?

Copy link
Contributor Author

@toddburnside toddburnside Sep 12, 2023

Choose a reason for hiding this comment

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

I have no problem switching to Ember, but I think I remember @tpolecat saying something about Ember vs Blaze so we should check with him.

Copy link
Member

@tpolecat tpolecat Sep 12, 2023

Choose a reason for hiding this comment

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

Ember was broken last time I tried it but you could give it a try. I would like to get off Blaze.

Copy link
Contributor Author

@toddburnside toddburnside Sep 12, 2023

Choose a reason for hiding this comment

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

One or more of the Armans have probably fixed it by now.

Copy link
Contributor

@armanbilge armanbilge Sep 12, 2023

Choose a reason for hiding this comment

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

Yeah at this point I'm making any remaining Ember bugs high priority. In the cutting-edge benchmarks its pulled ahead of Blaze.

Copy link
Contributor Author

@toddburnside toddburnside Sep 12, 2023

Choose a reason for hiding this comment

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

Do you want to switch in a separate PR?

Copy link
Contributor

@armanbilge armanbilge Sep 12, 2023

Choose a reason for hiding this comment

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

Yeah I can work on that. While I do it I will also double-check (and fix if needed) Ember's behavior in this situation.

Copy link
Contributor Author

@toddburnside toddburnside Sep 12, 2023

Choose a reason for hiding this comment

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

I could do the switch to ember if you want to look at the implementation.

Copy link
Contributor Author

@toddburnside toddburnside Sep 12, 2023

Choose a reason for hiding this comment

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

Actually, on my attempt I keep getting these

[ERROR] org.http4s.ember.server.EmberServerBuilderCompanionPlatform - WebSocket connection terminated with exception
java.util.concurrent.TimeoutException: 60 seconds

So I'm probably missing something...

.recoverWith {
case EntityLimiter.EntityTooLarge(_) =>
BadRequest(s"File too large. Limit of $maxUploadMb MB")
Expand All @@ -113,6 +115,7 @@ object ObsAttachmentRoutes {
s
.updateAttachment(user, programId, attachmentId, fileName, description, req.body)
.flatMap(_ => Ok())
.guarantee(req.body.compile.drain)
.recoverWith {
case EntityLimiter.EntityTooLarge(_) =>
Copy link
Contributor

@armanbilge armanbilge Sep 12, 2023

Choose a reason for hiding this comment

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

I recently fixed a bug in the EntityLimiter middleware but it's not released yet. I'm surprised if it hasn't been affecting you?

Copy link
Contributor Author

@toddburnside toddburnside Sep 12, 2023

Choose a reason for hiding this comment

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

Interesting. It seems to have been working. Of course on Heroku we just got a 503.

BadRequest(s"File too large. Limit of $maxUploadMb MB")
Expand Down
3 changes: 3 additions & 0 deletions modules/service/src/main/scala/lucuma/odb/graphql/ProposalAttachmentRoutes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package lucuma.odb.graphql

import cats.effect._
import cats.effect.implicits._
import cats.implicits._
import eu.timepit.refined.types.string.NonEmptyString
import lucuma.core.model.Program
Expand Down Expand Up @@ -95,6 +96,7 @@ object ProposalAttachmentRoutes {
s
.insertAttachment(user, programId, attachmentType, fileName, description, req.body)
.flatMap(_ => Ok())
.guarantee(req.body.compile.drain)
.recoverWith {
case EntityLimiter.EntityTooLarge(_) =>
BadRequest(s"File too large. Limit of $maxUploadMb MB")
Expand All @@ -111,6 +113,7 @@ object ProposalAttachmentRoutes {
s
.updateAttachment(user, programId, attachmentType, fileName, description, req.body)
.flatMap(_ => Ok())
.guarantee(req.body.compile.drain)
.recoverWith {
case EntityLimiter.EntityTooLarge(_) =>
BadRequest(s"File too large. Limit of $maxUploadMb MB")
Expand Down