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

Problem with unmarshaling #15

Closed
gchudnov opened this issue Aug 3, 2023 · 15 comments · Fixed by #93
Closed

Problem with unmarshaling #15

gchudnov opened this issue Aug 3, 2023 · 15 comments · Fixed by #93

Comments

@gchudnov
Copy link

gchudnov commented Aug 3, 2023

Hi @mdedetrich ,
it seems there is a problem in unmarshalling

JsonSupport.scala#L36

  implicit def jsonUnmarshaller[J: Facade]: FromEntityUnmarshaller[J] =
    Unmarshaller
      .withMaterializer[HttpEntity, J](_ =>
        implicit mat => {
          case HttpEntity.Strict(_, data) =>
            FastFuture(JsonStreamParser.parse[J](data))
          case entity => entity.dataBytes.runWith(JsonStreamParser.head[J]) // <-- HERE
        }
      )
      .forContentTypes(`application/json`)

and, for example:
main/scala/org/zalando/kanadi/api/Subscriptions.scala#L569-L570 { NOTE: the master-branch is still akka, to the point was migrating to pekko / building only locally }

never produces the result and never completes.

--
but if run manually, e.g.

val dataF = response.entity.dataBytes.runFold(ByteString.empty)(_ ++ _).map(_.utf8String)

and then decoding,

decode[Subscription](data)

there are no issues.

Guess it depends how the response is chunked and it might be not enough data in the .head.

@mdedetrich
Copy link
Owner

@gchudnov To clarify, is this a new issue due to pekko migration or has it always existed (i.e. with akka-streams-json that uses akka)?

@gchudnov
Copy link
Author

gchudnov commented Aug 5, 2023

yes, after migration.
the difference, is before it was using:

    "org.mdedetrich"             %% "akka-stream-circe"   % akkaStreamsJsonVersion,
    "org.mdedetrich"             %% "akka-http-circe"     % akkaStreamsJsonVersion,
    "de.heikoseeberger"          %% "akka-http-circe"     % heikoseebergerAkkaHttpCirceVersion,

and after migration:

libraryDependencies ++= List(
  "org.mdedetrich" %% "pekko-stream-circe" % "<VERSION>",
  "org.mdedetrich" %% "pekko-http-circe" % "<VERSION>"
)

it must be affected some Unmarshal[.] operations.

@mdedetrich
Copy link
Owner

mdedetrich commented Aug 5, 2023

What versions of akka/akka-streams-json were you using before?

@gchudnov
Copy link
Author

gchudnov commented Aug 5, 2023

2.6.20

@mdedetrich
Copy link
Owner

And the akka-streams-json version?

@gchudnov
Copy link
Author

gchudnov commented Aug 5, 2023

sorry, missed somehow in the question above :(
0.8.3

@mdedetrich
Copy link
Owner

Thanks, ill do my best to try and look into it this week

@mdedetrich
Copy link
Owner

@gchudnov I don't think I have time to look into it this week, if you want to pick up the issue then feel free to do so

@jrudolph
Copy link

The problem with using head is that it will cancel request body stream which will kill the HTTP connection. A similar problem was already discovered and fixed in play a while ago.

@mdedetrich
Copy link
Owner

mdedetrich commented Sep 12, 2024

The problem with using head is that it will cancel request body stream which will kill the HTTP connection. A similar problem was already discovered and fixed in play a while ago.

Maybe that explains it, because this issue did not occur in earlier versions of akka-streams, likely 2.5 which behaved differently wrt stream cancellation.

@mdedetrich
Copy link
Owner

I am going to close this issue as its a user error (i.e. an unhandled exception via .head being empty cancelling a stream to me falls under intended behaviour).

@jrudolph
Copy link

No, this is a bug in this library.

@mdedetrich
Copy link
Owner

mdedetrich commented Sep 12, 2024

Oh woops, I thought the code was internal and not this library. Re-opening and fixing now.

@mdedetrich
Copy link
Owner

@gchudnov Incase you are still using pekko-streams-circe, this issue has been resolved in version 1.1.0 which was just released (see https://github.com/mdedetrich/pekko-streams-circe/releases/tag/v1.1.0)

@gchudnov
Copy link
Author

@mdedetrich
great to hear, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants