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

http4s client tries to re-run body stream, possibly uses disposed connection #391

Open
fiadliel opened this issue Apr 3, 2018 · 6 comments

Comments

@fiadliel
Copy link
Contributor

fiadliel commented Apr 3, 2018

_executeRequest is defined like this:

    def _executeRequest[T, U](
      method: String,
      path: Seq[String],
      queryParameters: Seq[(String, String)] = Nil,
      requestHeaders: Seq[(String, String)] = Nil,
      body: Option[T] = None
    )(handler: org.http4s.Response[F] => F[U]
    )(implicit encoder: io.circe.Encoder[T]): F[U] = {

and uses the fetch method of the http4s client:

      httpClient.fetch(modifyRequest(authBody))(handler)

If we look at the scaladoc for fetch:

def fetch[A](req: Request[F])(f: (Response[F]) ⇒ F[A]): F[A]
Submits a request, and provides a callback to process the response.

req
The request to submit

f
A callback for the response to req. The underlying HTTP connection is disposed when the returned task completes. Attempts to read the response body afterward will result in an error.

returns
The result of applying f to the response to req

This means you should not access the body after this point in the code.

However, when an expected error occurs, the response is given back, including a call which tries to parse JSON in a lazy val. This is fundamentally unsafe for two reasons:

  1. the connection could already have been disposed before the lazy val is evaluated (which is outside of the fetch block).
  2. if the body has already been consumed, in a streaming situation, you cannot restart streaming. This might not have been done in some cases, but it's a fiddly thing to keep track of.
@mchimirev
Copy link
Collaborator

We ran into a case, most likely caused by this issue, where when we try to decode a large (49,084 characters) response we get an exception:
java.lang.RuntimeException: org.http4s.InvalidBodyException: Received premature EOF.

Function looks like this:

  override def getOrdersByOrderId(_req: Request[IO], organization: com.hbc.mobile.common.models.v0.models.Organization, orderId: Long, billingZipCode: String): IO[GetOrdersByOrderIdResponse] = {
    val response: IO[GetOrdersByOrderIdResponse] = client.accounts.getOrdersByOrderId(orderId, billingZipCode, getSessionId(_req)).map { accountOrderTrackingResponseWrapper =>
      GetOrdersByOrderIdResponse.HTTP200(accountOrderTrackingResponseWrapper.response.results.order.map(toAccountOrder).getOrElse {
        throw new RuntimeException("No order present")
      })
    }
    response.recoverWith {
      case wrapperResponse: client.errors.AccountOrderStatusResponseWrapperResponse => {
        wrapperResponse.response.status.code match {
          case 404 => {
            wrapperResponse.accountOrderStatusResponseWrapper.attempt.map {
              case Left(e) =>
                throw new RuntimeException(e)
              case Right(accountOrderStatusResponseWrapper) =>
                GetOrdersByOrderIdResponse.HTTP422(ValidationErrors(cleanupValidationErrors(accountOrderStatusResponseWrapper.errors.map(toValidationError))))
            }
          }
          case undefined =>
            throw new RuntimeException(s"undefined response code ${undefined}")
        }
      }
    }
  }

And we are getting a 404 back but we are not able to decode the response.

@amarrella If you could look at it tomorrow, that would be great, it's blocking some of our work.

@amarrella
Copy link
Collaborator

amarrella commented Apr 4, 2018

The best thing would be to return a fs2.Stream in all the functions, but that's a pretty big breaking change.
Second best thing would be to do something like this in _executeRequest:

httpClient
        .streaming(modifyRequest(authBody))(response => fs2.Stream.emit(handler(response)))
        .compile.foldMonoid.flatten

but this would add the requirement for F[U] to be a monoid (even if we build the proof in the generated code, this means that we need to require U to be a monoid at least). Again, this would be a breaking change.

I think the only option we have for now is replacing the fetch call with this:

      for {
        disposableResponse <- httpClient.open(request)
        handledResponse <- handler(disposableResponse.response)
        _ <- Sync[F].delay(disposableResponse.dispose)
      } yield handledResponse

@fiadliel what do you think?

@amarrella
Copy link
Collaborator

OK, bad news @mchimirev. After talking with @fiadliel we came to the conclusion that client is broken in this case and the only way to fix it is a pretty big breaking change (the ones suggested above won't solve the issue). My suggestion for now is to unfortunately not use the client, do the client manually and use the apibuilder ModelsJson.

@gheine @fiadliel any suggestion on how to move forward with this?

@gheine
Copy link
Collaborator

gheine commented Apr 4, 2018

So the problem is confined to non-unit error responses and their lazily parsing the response body. A quick fix could be to make the error type non-lazy, ie. deserialize when the error response case class is constructed, similar to what happens when non-error response types are deserialized.

@gheine
Copy link
Collaborator

gheine commented Apr 4, 2018

ie change from lazy val to val in:

case class ValidationErrorsResponse(
      response: org.http4s.Response[F],
      message: Option[String] = None
    ) extends Exception(message.getOrElse(response.status.code + ": " + response.body)){
      lazy val validationErrors = _root_.com.foo.v0.Client.parseJson[F, com.foo.v0.models.ValidationErrors]("com.foo.v0.models.ValidationErrors", response)
    }

amarrella added a commit to amarrella/apibuilder-generator that referenced this issue Apr 4, 2018
@mchimirev
Copy link
Collaborator

mchimirev commented Apr 4, 2018

Tested using mobile-middleware (see branch issue-391-fix), and every thing is working correctly. I especially love not having to .attempt. response from the client.

For example, this was old code to handle 409:

    response.recoverWith {
      case wrapperResponse: client.errors.UserMessageResponse => {
        wrapperResponse.response.status.code match {
          case 409 => {
            wrapperResponse.userMessage.attempt.map {
              case Left(e) =>
                throw new RuntimeException(e)
              case Right(userMessage) =>
                GetResponse.HTTP409(toUserMessage(userMessage))
            }
          }
          case undefined =>
            throw new RuntimeException(s"undefined response code ${undefined}")
        }
      }
    }

And this is new code:

    response.recover {
      case wrapperResponse: client.errors.UserMessageResponse => {
        wrapperResponse.response.status.code match {
          case 409 => {
            GetResponse.HTTP409(toUserMessage(wrapperResponse.userMessage))
          }
          case undefined =>
            throw new RuntimeException(s"undefined response code ${undefined}")
        }
      }
    }

amarrella added a commit that referenced this issue Apr 5, 2018
* Moved error models to client class and removed effect

* Added errors object within the client to avoid namespace conflicts

* Formatting

* Fixed error scope in client

* Parsing json body before returning. Solves #391

* Lifted body into F for backward compatibility and renamed decoded body to 'body'

* Inlined exceptionclass and simplified pattern match
anadoba pushed a commit to anadoba/apibuilder-generator that referenced this issue Mar 14, 2019
…#392)

* Moved error models to client class and removed effect

* Added errors object within the client to avoid namespace conflicts

* Formatting

* Fixed error scope in client

* Parsing json body before returning. Solves apicollective#391

* Lifted body into F for backward compatibility and renamed decoded body to 'body'

* Inlined exceptionclass and simplified pattern match
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants