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

v4 multiple frames in polling body #959

Merged
merged 11 commits into from
Feb 26, 2024

Conversation

unverbraucht
Copy link
Contributor

As mentioned in the other PR I still have this issue where a v4 client will send multiple requests in the body of one Polling HTTP request. Since only the first message is read the client times out waiting for ACK.
This PR adds parsing support for parsing multiple messages from one packet. I've also added a unit test for this since it was an easy way to test this. To use the new HTTP client in Java 9+ for the test I bumped Surefire to v3 (java 9+ support).

I also bumped general compile from Java 9 which is EOL to Java 11 which should still be supported.

@unverbraucht
Copy link
Contributor Author

The CI is failing because building would now need Java 11. If this is an issue let me know and I'll try to work around it. I think it's a fairly legit requirement these days though as it's already six years old by now :)
I made some more changes to the parsing so that it shouldn't interfere with binary protocol. I have not tested binary though as my application doesn't use it, maybe somebody else can test it.

@mrniko
Copy link
Owner

mrniko commented Jan 25, 2024

Java 8 is used by around 33% of application world wide. So this version should be supported. Java 11 for testing is fine.

@mrniko mrniko added this to the 2.0.9 milestone Jan 25, 2024
@unverbraucht
Copy link
Contributor Author

Yes, Java 8 is still supported by the JAR itself. But the test I wrote uses some Java 11 classes. I can try to replace it with another HTTP client or remove the test itself.

Run tests with Java 8.
@unverbraucht
Copy link
Contributor Author

I've refactored the test to run with Java 8. BTW I don't think that any tests are run at all, the tests are set to force skipped in the pom. At least I can't run them locally.

@unverbraucht
Copy link
Contributor Author

So tests run for JDK 17 and 21. JDK 8 cannot work with the current approach of having a module-info compiled with JDK 9+ and the rest of the JAR compiled with JDK 8 because we need at least JDK 9+ to compile the module-info. The same is true with all other approaches that want to achieve Java module support while still supporting JDK8. The only solution I can see is drop compile testing with JDK8.
I don't understand why JDK11 fails. Judging from the output it seems that the spring dependency is already only JDK17+ (major version 61). We'd have to either go to an older dependency version or drop JDK11 test builds as well.

BTW, do you want me to look into the tests? I enabled them locally but some of them fail, which is probably why they're disabled in the pom?

@mrniko
Copy link
Owner

mrniko commented Jan 26, 2024

@unverbraucht

JDK 17 is also fine for tests.

BTW, do you want me to look into the tests? I enabled them locally but some of them fail, which is probably why they're disabled in the pom?

I would appreciate it.

Explain about building with Java 11+ but JAR compatible with Java 8.
@unverbraucht
Copy link
Contributor Author

I looked into the build issues. For some reason spring beanutils 6.0 is used at compile time, which has a baseline of Java SDK 17+. The pom declares a dependency on spring beanutils 5.3 so it's unclear where this comes from. Could it be because it's scope is "provided"?
In any case I think the PR can still be merged, this is not a regression over master.

@unverbraucht
Copy link
Contributor Author

Ah now I saw it. Commit 4209e0f (dependabot bump of spring core) pulled in spring 6.0. @mrniko with this we now require JDK 17+ at runtime. If you want to keep the JDK8 baseline we should revert that version bump.

@mrniko
Copy link
Owner

mrniko commented Jan 31, 2024

@unverbraucht

This dependency has provided scope. While the project is compiled for JDK8.

@unverbraucht
Copy link
Contributor Author

unverbraucht commented Jan 31, 2024 via email

@mrniko mrniko merged commit 57f3de3 into mrniko:master Feb 26, 2024
2 checks passed
@mrniko
Copy link
Owner

mrniko commented Feb 26, 2024

Thanks for contribution!

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.

None yet

2 participants