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

improve WS stream, add `onClose` handler #1973

Merged
merged 2 commits into from Aug 7, 2018

Conversation

@jmcardon
Copy link
Member

@jmcardon jmcardon commented Aug 3, 2018

This PR should optimize some of the websocket stream code, as well as providing clients a finalizer to run when the websocket closes.

Breaking down the old inputStream:

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

Problems with this code:

  • We do not need to end the stream because we decided to stop sending elements. we can choose to not send any more on the server. If anything, we can also give a handler to shutting down manually if necessary, and I can add this.
  • onStreamFinalize is a moot check: the stream will be interrupted regardless once it receives a close frame since that will set the signal.
  • onClose can be added as a finalizer before sendClose

Similarly:

    unsafeRunAsync {
      wsStream.attempt.flatMap {
        case Left(_) => sendClose
        case Right(_) => ().pure[F]
      }
    } {
      case Left(t) =>
        IO(logger.error(t)("Error closing Web Socket"))
      case Right(_) =>
        // Nothing to do here
        IO.unit
    }
  }

The left case will almost surely fail, as sendClose is guaranteed to be run by the finalizer.

We are lacking websocket tests. I'm unsure whether to add them in this PR.

Copy link
Member

@rossabaker rossabaker left a comment

👍 LGTM

@ChristopherDavenport ChristopherDavenport merged commit 44b30da into http4s:master Aug 7, 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

3 participants
You can’t perform that action at this time.