-
Notifications
You must be signed in to change notification settings - Fork 787
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.
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
Highlight the uncancelable behavior in Timeout
middleware scaladoc
#6407
Conversation
server/shared/src/main/scala/org/http4s/server/middleware/Timeout.scala
Outdated
Show resolved
Hide resolved
server/shared/src/main/scala/org/http4s/server/middleware/Timeout.scala
Outdated
Show resolved
Hide resolved
server/shared/src/main/scala/org/http4s/server/middleware/Timeout.scala
Outdated
Show resolved
Hide resolved
server/shared/src/main/scala/org/http4s/server/middleware/Timeout.scala
Outdated
Show resolved
Hide resolved
Thanks, sorry for the nitpicks. I think this is a really important topic to cover. |
test( | ||
"return a 503 error if the result takes too long and execute underlying uncancelable effect" | ||
) { | ||
testMiddleware(5.milliseconds) { app => | ||
for { | ||
_ <- app(uncancelableReq).map(_.status).assertEquals(Status.ServiceUnavailable) | ||
_ <- checkStatus(app(uncancelableReq), Status.ServiceUnavailable) | ||
.intercept[TimeoutException] | ||
} yield () | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm a little confused what this test is demonstrating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- it shows that uncancelable effects complete fully without any cancelation (from middleware) in a case when a timeout has exceeded (test at the 90 line)
- it shows that
Timeout
middleware will respond with the timeout response anyway (test at the 89 line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you for explaining, I think I finally understood. The test for (1) is the .intercept[TimeoutException]
because 1.seconds < IO.sleep(1100.milliseconds)
IIUC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it so, you're correct.
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I hope you don't mind, I added links to the CE API docs and confirmed that they work as expected.
I think the uncancelable behavior is noteworthy because it could lead to some surprising effects on the user site. It's indisputable that it's the user's responsibility to check how their services produce effects. But still, I'm sure it's better to highlight that behavior.
Also, I've added the test that proves my point about
Timeout
middleware behavior when dealing with uncancelable effects.