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

test with Pekko 1.1 milestone releases #12662

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pjfanning
Copy link
Contributor

Pull Request Checklist

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
Copy link
Contributor Author

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
Copy link
Member

mkurz commented May 30, 2024

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
Copy link
Contributor Author

pjfanning commented May 30, 2024

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
Copy link
Contributor

gwak commented Jul 7, 2024

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:

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

should now be:

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

@gwak
Copy link
Contributor

gwak commented Jul 7, 2024

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

@pjfanning
Copy link
Contributor Author

@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
Copy link
Contributor

gwak commented Jul 9, 2024

@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 ?

@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
Copy link
Contributor Author

@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
Copy link
Member

mkurz commented Jul 9, 2024

@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.

@mkurz

This comment was marked as resolved.

This comment was marked as resolved.

@pjfanning
Copy link
Contributor Author

pjfanning commented Sep 4, 2024

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

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

Successfully merging this pull request may close these issues.

3 participants