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

Websocket: Update To Unsafe Signal for Close Rather than Pure Signal #1631

Merged

Conversation

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jan 17, 2018

No description provided.

Copy link
Member

@rossabaker rossabaker left a comment

👍 Seems legit. I assume this has been tested manually.

Copy link
Member

@bryce-anderson bryce-anderson left a comment

I'm not actually up to date on my streaming combinators so I can't provide feedback on the changes to wsStream but the rest looks good to me.

@@ -80,18 +74,20 @@ class Http4sWSStage[F[_]](ws: ws4s.Websocket[F])(implicit F: Effect[F], val ec:
val onStreamFinalize: F[Unit] =
for {
dec <- F.delay(count.decrementAndGet())
_ <- deadSignal.map(signal => if (dec == 0) signal.set(true))
_ <- if (dec == 0) deadSignal.set(true) else ().pure[F]

This comment has been minimized.

@SystemFw

SystemFw Jan 17, 2018
Member

().pure[F] --> F.unit?

nicer, but utterly unimportant ofc

val wsStream = inputstream
.to(ws.receive)
.onFinalize(onStreamFinalize)
.mergeHaltR(ws.send.onFinalize(onStreamFinalize).to(snk).drain)

This comment has been minimized.

@SystemFw

SystemFw Jan 17, 2018
Member

foo.mergeHaltR(bar.drain) can probably be bar.concurrently(foo) now, but with the new merge on the way this should work as well

This comment has been minimized.

@SystemFw

SystemFw Jan 17, 2018
Member

Actually scratch that sorry, you nee the output of foo, but want to complete when bar is done

@SystemFw
Copy link
Member

@SystemFw SystemFw commented Jan 17, 2018

Left a few style comments, but nothing too important. I'm also assuming this has been tested

Copy link
Member

@rossabaker rossabaker left a comment

👍, though I like @SystemFw's suggestions

@SystemFw
Copy link
Member

@SystemFw SystemFw commented Jan 17, 2018

@ChristopherDavenport you were too fast!

@SystemFw
Copy link
Member

@SystemFw SystemFw commented Jan 17, 2018

so I was imprecise, concurrently replaces mergeHaltL not mergeHaltR, so if you indeed want to finish the right branch finishes (but still return the output of the right branch), you indeed need what you used to have (sorry 😁)

@ChristopherDavenport
Copy link
Member Author

@ChristopherDavenport ChristopherDavenport commented Jan 17, 2018

:(

@ChristopherDavenport ChristopherDavenport changed the title Websocket: Update To Use Atomic Reference for Close Rather than Pure Signal Websocket: Update To Unsafe Signal for Close Rather than Pure Signal Jan 17, 2018
@ChristopherDavenport ChristopherDavenport merged commit 18d659e into http4s:master Jan 17, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ChristopherDavenport ChristopherDavenport deleted the ChristopherDavenport:updateWSStage branch Jan 17, 2018
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

5 participants