Prevent 503's on attachment uploads #709
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ember was broken last time I tried it but you could give it a try. I would like to get off Blaze.
There was a problem hiding this comment.
One or more of the Armans have probably fixed it by now.
There was a problem hiding this comment.
Yeah at this point I'm making any remaining Ember bugs high priority. In the cutting-edge benchmarks its pulled ahead of Blaze.
There was a problem hiding this comment.
Do you want to switch in a separate PR?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I could do the switch to ember if you want to look at the implementation.
There was a problem hiding this comment.
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...
| @@ -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(_) => | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Interesting. It seems to have been working. Of course on Heroku we just got a 503.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #709 +/- ##
==========================================
+ Coverage 78.77% 79.02% +0.24%
==========================================
Files 507 507
Lines 9203 9296 +93
Branches 23 28 +5
==========================================
+ Hits 7250 7346 +96
+ Misses 1953 1950 -3
☔ View full report in Codecov by Sentry. |
As noted in #528, Heroku has the unfortunate behavior of returning a 503 if you try to send a response before reading the entire body of a request. So, this PR always drains the request stream before responding.