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

Remove try/catch semantics in otherwise pure code #1592

Merged
merged 9 commits into from Dec 22, 2017

Conversation

@jmcardon
Copy link
Member

@jmcardon jmcardon commented Dec 11, 2017

Handling #1578 and cleaning up code in general.

@@ -26,8 +26,7 @@ final case class DisposableResponse[F[_]](response: Response[F], dispose: F[Unit
* HTTP connection when the task finishes.
*/
def apply[A](f: Response[F] => F[A])(implicit F: MonadError[F, Throwable]): F[A] = {
val task = try f(response)
catch { case e: Throwable => F.raiseError[A](e) }
val task = f(response)

This comment has been minimized.

@jmcardon

jmcardon Dec 14, 2017
Author Member

This is specifically the change I want to discuss.

I'm unsure whether catching this is.. good. It's a sort of cushion but I do not think this should be necessary if the result is parametrized to begin with.

This comment has been minimized.

@rossabaker

rossabaker Dec 15, 2017
Member

An exception here will leak a connection, which is a severe response to a newbie mistake. A try that is uncaught is negligible, to possibly zero, performance overhead. (I've been told both and not measured myself.) I think this might be a good case to lecture in the logs but try to keep the system operational.

This comment has been minimized.

@jmcardon

jmcardon Dec 15, 2017
Author Member

Fair.. I just wanted to discuss it and leave a note there in the case we do not remove it.

@jmcardon jmcardon requested a review from rossabaker Dec 14, 2017
@@ -26,8 +26,7 @@ final case class DisposableResponse[F[_]](response: Response[F], dispose: F[Unit
* HTTP connection when the task finishes.
*/
def apply[A](f: Response[F] => F[A])(implicit F: MonadError[F, Throwable]): F[A] = {
val task = try f(response)
catch { case e: Throwable => F.raiseError[A](e) }
val task = f(response)

This comment has been minimized.

@rossabaker

rossabaker Dec 15, 2017
Member

An exception here will leak a connection, which is a severe response to a newbie mistake. A try that is uncaught is negligible, to possibly zero, performance overhead. (I've been told both and not measured myself.) I think this might be a good case to lecture in the logs but try to keep the system operational.

*/
def decodeBy[F[_]: Applicative, T](r1: MediaRange, rs: MediaRange*)(
def decodeBy[F[_]: Sync, T](r1: MediaRange, rs: MediaRange*)(

This comment has been minimized.

@rossabaker

rossabaker Dec 15, 2017
Member

The DecodeResult still lets us raiseError, no?

This comment has been minimized.

@jmcardon

jmcardon Dec 15, 2017
Author Member

I guess but raiseError on EitherT doesn't result in the same as F.raiseError, I think.

This comment has been minimized.

@jmcardon

jmcardon Dec 15, 2017
Author Member

Should we not tighten this to a sync bound? I'm just not to keen on just Applicative possibly denoting something impure (which people can do in the f

This comment has been minimized.

@rossabaker

rossabaker Dec 16, 2017
Member

A failure to decode should be a DecodeResult.failure, and people can write an f that throws regardless of the type bound. How is that stronger constraint going to be used?

@@ -63,6 +63,7 @@ package object util {
new AssertionError(
s"This is a bug. Please report to https://github.com/http4s/http4s/issues: ${message}")

// Todo: Possibly deprecate? Cats already has this.

This comment has been minimized.

@rossabaker

rossabaker Dec 15, 2017
Member

It's private and totally redundant. I'd straight up remove this one.

@@ -56,6 +56,7 @@ object ParseResult {
def success[A](a: A): ParseResult[A] =
Either.right(a)

//Todo: Possibly deprecate???

This comment has been minimized.

@rossabaker

rossabaker Dec 15, 2017
Member

I think it's a legit construction from unsafe code. It's not necessary, but I guess preserves the aliasing of Either to a ParseResult. I'm inclined to keep this one.

} catch { case t: Throwable => handleException(t); facc }
}
.apply(newReq)
.getOrElse(Vector.empty[PushResponse[F]])

This comment has been minimized.

@rossabaker

rossabaker Dec 15, 2017
Member

I'm trying to think what exception here we were motivated to catch in the first place.

This comment has been minimized.

@jmcardon

jmcardon Dec 15, 2017
Author Member

I Don't think this one is doing anything in particular.. its removal didn't break anything.

This comment has been minimized.

@rossabaker

rossabaker Dec 16, 2017
Member

Kill it.

@@ -96,18 +96,11 @@ class Http4sServlet[F[_]](
request: Request[F],
bodyWriter: BodyWriter[F]): F[Unit] = {
ctx.addListener(new AsyncTimeoutHandler(request, bodyWriter))
// TODO: This can probably be handled nicer with Effect
// Note: We're catching silly user errors in the lift => flatten.

This comment has been minimized.

@rossabaker

rossabaker Dec 15, 2017
Member

This is a case where I'd be happy to lecture about the silliness.

There is a similar construction in the blaze Http1ServerStage.

This comment has been minimized.

@jmcardon

jmcardon Dec 16, 2017
Author Member

the one in the Http1ServerStage is of the form:

  executionContext.execute(new Runnable {
          def run(): Unit =
            F.runAsync {
                try serviceFn(req)
                  .getOrElse(Response.notFound)
                  .handleErrorWith(serviceErrorHandler(req))
                catch serviceErrorHandler(req)
              } {
                case Right(resp) =>
                  IO(renderResponse(req, resp, cleanup))
                case Left(t) =>
                  IO(internalServerError(s"Error running route: $req", t, req, cleanup))
              }
              .unsafeRunSync()
        })

This is similar to writing fs2.async.unsafeRunAsync(F)(callback) but with the exception that F.shift(ec) submits a task onto the execution context.

Maybe im being a paranoid panda but while the ServerStage1 is uglier, it might be more efficient (In a sense, it's not submitting a useless task like the shift method does to evaluate the continuation) and I'm afraid of a tiny performance regression because of a change there.

Shift defined (In IO) as:

  def shift(implicit ec: ExecutionContext): IO[Unit] = {
    IO async { (cb: Either[Throwable, Unit] => Unit) =>
      ec.execute(new Runnable {
        def run() = cb(Right(()))
      })
    }
  }

I actually could see what's on master to be more efficient, albeit ugly to look at.

Just my 2 cents. I'm probably microoptimizing for nothing so a second perspective would be nice.

This comment has been minimized.

@rossabaker

rossabaker Dec 16, 2017
Member

That's one of the hotspots in a ping test, which sadly is how people choose libraries. (Shakes fist at naive reactions to Techempower.) Okay, let's be conservative there, but the sanctimonious logging could be the same.

@jmcardon jmcardon added freezer and removed freezer labels Dec 15, 2017
val task: F[A] = try f(response)
catch {
case e: Throwable =>
logger.info(s"Caught exception in otherwise pure expression $e")

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Dec 16, 2017
Member

I think we can lecture stronger than this. logger.error perhaps.

@@ -33,7 +33,7 @@ final case class DisposableResponse[F[_]](response: Response[F], dispose: F[Unit
val task: F[A] = try f(response)
catch {
case e: Throwable =>
logger.info(s"Caught exception in otherwise pure expression $e")
logger.error(s"Caught exception in otherwise pure expression $e")

This comment has been minimized.

@rossabaker

rossabaker Dec 16, 2017
Member

We don't know it was otherwise pure. And I'm not sure this tells a new FP programmer what they did wrong.

logger.error(e)("Handled exception in client callback to prevent a connection leak. The callback should always return an F. If your callback can fail with an exception you can't handle, call F.raiseError(exception)`.")

jmcardon added 2 commits Dec 22, 2017
@rossabaker rossabaker changed the title (WIP) remove try/catch semantics in otherwise pure code Remove try/catch semantics in otherwise pure code Dec 22, 2017
rossabaker and others added 2 commits Dec 22, 2017
@@ -122,25 +121,21 @@ object EntityDecoder extends EntityDecoderInstances {
/** Create a new [[EntityDecoder]]
*
* The new [[EntityDecoder]] will attempt to decode messages of type `T`
* only if the [[Message]] satisfies the provided [[MediaRange]]s
* only if the [[Message]] satisfies the provided [[MediaRange]]
* note: Sync bound to note that such an `F[_]` may throw

This comment has been minimized.

@aeons

aeons Dec 22, 2017
Member

Isn't the bound still Applicative or is it referring to something else?

This comment has been minimized.

@rossabaker

rossabaker Dec 22, 2017
Member

Good catch. I took a stab at correcting it.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Dec 22, 2017

For expediency, gambling that Travis likes this one since it liked the last one except for scalafmt.

@rossabaker rossabaker merged commit aff7a46 into http4s:master Dec 22, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
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
You can’t perform that action at this time.