Pekko 1.2, Pekko-Http 1.3, Jackson 2.20

[pjfanning]

Pull Request Checklist

• Have you read How to write the perfect pull request?
• Have you read through the contributor guidelines?
• Have you referenced any issues you're fixing using commit message keywords?
• Have you added copyright headers to new files?
• Have you checked that both Scala and Java APIs are updated?
• Have you updated the documentation for both Scala and Java sections?
• Have you added tests for any changed functionality?

Helpful things

Fixes

Fixes #xxxx

Purpose

To check on whether Play builds cleanly with Pekko 1.1.0-M1

What does this PR do?

Background Context

Why did you take this approach?

References

Are there any relevant issues / PRs / mailing lists discussions?

[pjfanning]

Issues in these tests.

[error] 	play.api.libs.ws.ahc.AhcWSSpec
[error] 	play.api.data.FormUtilsSpec
[error] 	play.it.libs.PekkoHttpJavaWSSpec
[error] 	play.it.http.parsing.MultipartFormDataParserSpec
[error] 	play.it.libs.PekkoHttpScalaWSSpec
[error] 	play.it.http.parsing.JacksonJsonBodyParserSpec
[error] 	play.it.action.FormActionSpec

[mkurz]

Thanks, this will be definitely looked at 👍
(I already knew that you upgraded jackson and that is causing problems in Play, the other stuff I am not sure yet)

[pjfanning]

I had a look at one test. play.api.libs.ws.ahc.AhcWSSpec

The test case that fails involves using ahc-client to send a multipart message based on 2 sources (representing a data part and a file part). The server side receives the request but the file part parsed data is incomplete. It seems like the ahc client sends the right request data but it's possible that there could be boundary or whitespace issues that prevent the multipart parser from handling the request properly.

[gwak]

I had a look at one test. play.api.libs.ws.ahc.AhcWSSpec

The test case that fails involves using ahc-client to send a multipart message based on 2 sources (representing a data part and a file part). The server side receives the request but the file part parsed data is incomplete. It seems like the ahc client sends the right request data but it's possible that there could be boundary or whitespace issues that prevent the multipart parser from handling the request properly.

From what I understand from the Pekko migration guide, we now need to specify the Pekko stream supervision strategy explicitly when using the splitWhen function.

the following code:

playframework/core/play/src/main/scala/play/core/parsers/Multipart.scala

Line 76 in 7abd258

.splitWhen(_.isLeft)

.splitWhen(_.isLeft)
.prefixAndTail(1)

should now be:

.splitWhen(_.isLeft)
.withAttributes(ActorAttributes.supervisionStrategy(Supervision.resumingDecider))
.prefixAndTail(1)

[gwak]

@pjfanning There's another occurence of that function here:

playframework/web/play-filters-helpers/src/main/scala/play/filters/csrf/CSRFActions.scala

Line 187 in 7abd258

.splitWhen(_ => false)

[pjfanning]

@gwak @mkurz I'm not sure if #12793 has helped. The tests still seem broken. #12793 is probably ok to keep but there might be more to the pekko 1.1 issues than that.

[gwak]

@gwak @mkurz I'm not sure if #12793 has helped. The tests still seem broken. #12793 is probably ok to keep but there might be more to the pekko 1.1 issues than that.

Hi @pjfanning I just had a look into this and tested with the 1.1.x Pekko release. This is what I tried but did not pass the MultiFormDataParserSpec:

.splitWhen(_.isLeft)
.withAttributes(ActorAttributes.supervisionStrategy(Supervision.resumingDecider))
.prefixAndTail(1)

But when using the deprecated SubstreamCancelStrategy it works fine:

.splitWhen(SubstreamCancelStrategy.drain)(_.isLeft)

So it seems that either the migration guide does not fully explain how to get the old behaviour back or there're still unresolved bugs in Pekko 1.1 ?

• Support Supervision.restart for SubFlow's apache/pekko#981
• Fix uncaught decider exception in Split with Supervision.resumingDecider apache/pekko#1207
• Replace SubstreamCancelStrategy with SupervisionDecider for Split apache/pekko#252
• SubFlow's don't correctly propagate Supervision.resumeStrategy apache/pekko#1205

@mkurz Maybe we should just use the "deprecated" way of setting the cancel strategy for now until it's resolved in Pekko ?

.splitWhen(SubstreamCancelStrategy.drain)(_.isLeft)

[pjfanning]

@gwak @mdedetrich b447af5 fixed a lot of tests. There are still some other failures but I think at least one of them relates to upgrading Jackson (so unrelated to the internal Pekko changes).

[mkurz]

@gwak @mdedetrich b447af5 fixed a lot of tests. There are still some other failures but I think at least one of them relates to upgrading Jackson (so unrelated to the internal Pekko changes).

yes the failing tests having nothing to do with pekko anymore, they are all about the jackson upgrade.

[pjfanning]

Pekko 1.1.0 is out. Pekko HTTP 1.1.0 is not yet released but could possibly be released within a couple of weeks.

The main thing blocking this is the fact that Play does not support the recent versions of Jackson - and Pekko 1.1.0 uses Jackson 2.17.

[error] 	play.api.data.FormUtilsSpec
[error] 	play.it.http.parsing.JacksonJsonBodyParserSpec

[pjfanning]

@mkurz this PR is a bit stale but it has a few elements that might be useful to work on after upgraded play-json is released

• there is some Jackson code in this repo too and it needs changing to fix test issues
  • core/play/src/main/java/play/libs/Json.java - this PR sets the StreamReadConstraints for that class to be max values but it may make sense to make it configurable too
• there is a Pekko enhancements for ByteString that has a more performant way to get an InputStream (this PR has the changes for Play to uptake that)

[mkurz]

but it may make sense to make it configurable too

If you want to add that to this PR that would be great (if you have the resources to do so)

[mkurz]

Finally. I feel very comfortable merging this PR now - specially because of the scripted test I added (plus I digged into this and play-ws, play-json and Jackson code very intensely).

I reverted some of the ideas from @pjfanning regarding on how to configure the objectmapper via hocon, most importantly because ConfigFactory.load was used to load a config - which in Play is not correct, because you always should get the config from a running Play application (which can build the config in who knows which way).
But even more important, before this PR, we had multiple objectmappers getting created - some of them hardcoded, totally unable to configure. With this PR (assuming ObjectMapperModule is enabled, which is the default anyway), we just have one single ObjectMapper instance across Play, play-json and play-ws anymore.
And this single ObjectMapper instance gets supplied by Pekko and can therefore be configued via the pekko.serialization.jackson.play {} config key. Plus: A Play app can now also configure the Play JSON standalone specific settings via play.json {} (which when using Play JSON standalone can only be set via system properties).

To make sure everything works as supposed to, the scripted test nails down the jackson config in various scenarios.
Another good thing about this scripted test is we now see on each jackson version bump which Jackson configs changed, got added and/or removed. To be honest because of that I feel much more comfortable now to upgrade (minor 2.x) Jackson even in a Play patch release (until now I was worried something might break, because Jackson is kind of a black whole for me). Also I think it will help us when upgrading to Jackson 3 one day.

I hope with this refactoring we can close the Jackson chapter for a while now (to be honest it seems it was a bit a messy before, most likely because play-json and play-ws was extraced from Play and were cooking their own soup).