Skip to content
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

Use race against the AsyncContext timeout #1802

Merged
merged 8 commits into from Aug 15, 2018

Conversation

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 24, 2018

Races the service against the async servlet context timeout.

  • Eliminates a verbose and unsafe callback.
  • Cancels a losing, potentially expensive response.
@rossabaker rossabaker requested a review from jmcardon Apr 24, 2018
@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Apr 24, 2018

There is more we can do with Concurrent. This is my simple example to spur discussion. @jmcardon has expressed worry about it due to the inability to implement it in scalaz8 IO.

I think it's a non-starter to forgo the benefits of Concurrent altogether, and highly unpalatable to use something like the Priority pattern, requiring two paths. My hope is that all the effects people want to use can be made Concurrent, either through clever implementation or negotiation on the laws. Time is running very short on the latter for cats-effect-1.0.

Copy link
Member

@aeons aeons left a comment

The new implementation looks a lot nicer.

renderResponse(response, servletResponse, bodyWriter)
F.race(timeout, response).flatMap {
case Left(()) =>
// The F.never is so we don't interrupt the rendering of the timeout response

This comment has been minimized.

@jcranky

jcranky Apr 25, 2018
Contributor

Which F.never?

This comment has been minimized.

This comment has been minimized.

@jcranky

jcranky Apr 25, 2018
Contributor

Ok, but from the comment I thought is was already being used here, and I don't see it anywhere 😀

This comment has been minimized.

@rossabaker

rossabaker Apr 25, 2018
Author Member

Yeah. I wrote the comment before I coded the F.never and remembered we don't have it yet. Consider it a TODO to replace it. :)

This comment has been minimized.

@jmcardon

jmcardon Apr 25, 2018
Member

We should leave it in IMHO. It will be never very soon.

Copy link
Member

@jmcardon jmcardon left a comment

So far this looks good to me.

Note re: ConcurrentEffect: Race is part of scalaz8 IO. It's definitely expressible in terms of it, it's the cancellable business that is not. The problem I have is with anything built off of cancellable and runCancellable, being that I still do believe it conflates resource safety with asynchrony really hard.

renderResponse(response, servletResponse, bodyWriter)
F.race(timeout, response).flatMap {
case Left(()) =>
// The F.never is so we don't interrupt the rendering of the timeout response

This comment has been minimized.

@jmcardon

jmcardon Apr 25, 2018
Member

We should leave it in IMHO. It will be never very soon.

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented May 30, 2018

Hell. This one is broken by #1887, too.

@rossabaker rossabaker added this to the 0.19.0-M2 milestone Aug 12, 2018
@rossabaker rossabaker force-pushed the rossabaker:concurrent-effect branch from ca8e391 to 64c1215 Aug 12, 2018
@rossabaker rossabaker dismissed aeons’s stale review Aug 12, 2018

Made some changes due to bitrot.

@aeons
aeons approved these changes Aug 15, 2018
@rossabaker rossabaker merged commit 03304e0 into http4s:master Aug 15, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants