API updates to support Java SE 17

[njr-11]

Remove Java SE 21 usage from API so that it will be possible to compile against Java SE 17 as is being required by the platform.

[mswatosh]

LGTM

[aubi]

When running on an earlier Java SE level, a value of {@code true} must result in an error such that the {@link ManagedExecutorService} is not created.

Why? Even on 21 this is optional, e.g. ignored? The only failing combination should be JDK 17 and value=true?
I suggest

When running on an earlier Java SE level, a value of {@code true} is ignored.

[aubi]

Let's agree on behavior of virtual=true on JDK < 21.

[njr-11]

Let's agree on behavior of virtual=true on JDK < 21.

Yeah, this is a good topic to discuss.

My reasoning for rejecting virtual=true on Java SE 17 was that an application should never be requesting virtual threads on Java SE 17 where it cannot possibly ever get virtual threads. Rather than hiding this information and continuing on, the user is better served by being made aware immediately so that they can make the decision to either switch to Java SE 21 or otherwise switch to write their application with a programming model that assumes platform threads.

What would be the reasoning behind the proposal to ignore virtual=true on Java SE 17? I suppose it would let you deploy the same application with virtual=true to both Java SE levels, but to me that seems like a disservice to the user when we know in advance that one of them is coded to the wrong programming model.

[starksm64]

Rejecting virtual=true on SE 17 makes sense to me. So there would be a tck test that validates that such a deployment fails when run on SE 17?

[njr-11]

Rejecting virtual=true on SE 17 makes sense to me. So there would be a tck test that validates that such a deployment fails when run on SE 17?

Right, whatever the outcome of this issue is, the TCK will need to take that into account in asserting what is the expected behavior.

[OndroMih]

Well, I don't see a reason why the spec should mandate that virtual=true raises an error when virtual threads are not supported by the JVM. If an app is written to run on Java 21+, then will probably be also compiled with Java 21 and won't even run on Java < 21. If the app has target level 17, then the programmers would very likely avoid virtual=true.

Therefore I suggest to keep things simple in the spec and TCK and just leave it to implementations how they react, whether they just ignore virtual=true and use platform threads, with or without a warning, or deployment fails. There's no point in specifying this edge case. I can't even see how an error with virtual=true would work for build-time frameworks like Quarkus, which would have to fail at build time I guess, when compiled with target level 17. It's pretty complicated to specify the valid behavior, and to create a universal test in the TCK for it, don't you think?

[OndroMih]

I suggest to change this to something like behavior is unspecified when virtual threads are not supported by the JVM.

[Emily-Jiang]

I am with @OndroMih on this one to use the category of the behavior is unspecified or non-portable behavior might occur. I think it is wrong to throw an error because true is Not an invalid value. Even in Java 21, JVM might not give you a VT. Nothing is wrong for asking. The worst case scenario is not to have one even though you have asked one when running at JDK 21-.

[aubi]

We agreed, that the implementation can create VTs.

If I understand correctly, the new suggested behavior, if the implementation decides NOT to create VTs, is:

virtual=false, JDK 21 - OK
virtual=true, JDK 21 - OK
virtual=false, JDK 17 - OK
virtual=true, JDK 17 - ERROR!

Why such artificial error? There is no hard expectation, the MES will produce Virtual Threads. It just can do it.
If the programmer wants to prevent running it on JDK 17, he can compile it for 21.

[njr-11]

Thanks for all of the comments. It is clear at this point that there is no agreement on a behavior for the combination of virtual=true with Java SE 17 and we now have a third option to consider with the approach of leaving it unspecified. That also means it will be untestable, which isn't as simple as skipping individual tests. The TCK will need to avoid deployment of entire test applications (any with virtual=true) on Java SE 17. The TCK client can do this, but as pointed out by Kyle doesn't currently have any way of knowing if the level of Java SE upon which the client is running will match what the tested Jakarta EE product is running on, so we will need to add a new requirement to the TCK requiring the same to be used for both.

I noticed that main description of this pull received a thumbs down vote,

Remove Java SE 21 usage from API so that it will be possible to compile against Java SE 17 as is being required by the platform.

Please note adding Java SE 17 support is not something we are doing because we want to. We are doing it because we are being forced to by the platform. We don't have a choice in the matter.

[OndroMih]

I added the downvote. It’s not against the description but against the changes suggested in this PR.

It’s also premature to address Java 17 support now, since the platform has not made the decision yet, it’s still under a ballot and it seems there are now more votes against than for.

[njr-11]

It’s also premature to address Java 17 support now, since the platform has not made the decision yet, it’s still under a ballot and it seems there are now more votes against than for.

Let's bring this up on the platform call tomorrow. It is on that call that they have been telling me that the matter is already decided and that we must make changes to support Java 17.

[Emily-Jiang]

Thanks for all of the comments. It is clear at this point that there is no agreement on a behavior for the combination of virtual=true with Java SE 17 and we now have a third option to consider with the approach of leaving it unspecified. That also means it will be untestable, which isn't as simple as skipping individual tests. The TCK will need to avoid deployment of entire test applications (any with virtual=true) on Java SE 17. The TCK client can do this, but as pointed out by Kyle doesn't currently have any way of knowing if the level of Java SE upon which the client is running will match what the tested Jakarta EE product is running on, so we will need to add a new requirement to the TCK requiring the same to be used for both.

I thought the same jvm will be used to run the test and start the runtime but they are different java instances. Have this been confirmed?

I noticed that main description of this pull received a thumbs down vote,

Remove Java SE 21 usage from API so that it will be possible to compile against Java SE 17 as is being required by the platform.

Please note adding Java SE 17 support is not something we are doing because we want to. We are doing it because we are being forced to by the platform. We don't have a choice in the matter.

[KyleAure]

@Emily-Jiang

I thought the same jvm will be used to run the test and start the runtime but they are different java instances. Have this been confirmed?

That would be the easiest way to run the TCK, but there is currently no requirement to do so.
Arquillian just connects to a running platform, but that platform does not need to be started by the project that run's the TCK. (In fact, when we run this TCK in Open Liberty our test infrastructure starts the server, not the tck-runner)

[OndroMih]

I already responded on the mailing list, but I see that the discussion continues here, so I copy it here:

I think that we should minimize the impact of the decision to support Java 17 as much as possible, even if the reaction to this would not be very popular. Therefore I suggest that we change the existing virtual thread tests in the TCK so that they don't validate anything about virtual threads. They should just ensure that an app with virtual=true can be deployed and runs as expected, regardless of whether platform or virtual threads are used. Then the tests would pass both on Java 17 and 21.

There's not value in making things perfect under these conditions. Supporting Java 17 is an additional work to do, and with limited resources, we have to be realistic and implement compromises.

[KyleAure]

@OndroMih

Therefore I suggest that we change the existing virtual thread tests in the TCK so that they don't validate anything about virtual threads. They should just ensure that an app with virtual=true can be deployed and runs as expected, regardless of whether platform or virtual threads are used. Then the tests would pass both on Java 17 and 21.

There are no existing virtual thread tests in the TCK.
The TCK will be written to pass Java 17 and Java 21 no one has ever suggested otherwise.
The assertions/assumptions made by the TCK will depend on what the agreed upon behavior is when using virtual=true on Java 17 which hasn't been agreed upon and why no tests have been added to the TCK.

[OndroMih]

There are no existing virtual thread tests in the TCK.
The TCK will be written to pass Java 17 and Java 21 no one has ever suggested otherwise.
The assertions/assumptions made by the TCK will depend on what the agreed upon behavior is when using virtual=true on Java 17 which hasn't been agreed upon and why no tests have been added to the TCK.

OK, so what does this PR actually solve? I think it's not necessary and everything should work with Java 17 even without this PR, right? Can we close it?

[njr-11]

OK, so what does this PR actually solve? I think it's not necessary and everything should work with Java 17 even without this PR, right? Can we close it?

I tried to combine two things into one pull, which is rarely ever a good idea.

The first area covered is being able to compile against Java SE 17 rather than Java SE 21. This required removing JavaDoc links that directly referenced Java SE 21 virtual thread API methods.

The second area covered is specifying the behavior to expect when Java SE 17 is used and virtual=true. It was a mistake on my part to propose something first in the pull rather than starting a community discussion and implementing the agreed upon answer. Now the pull is stuck waiting for the community to vote on an outcome so that it can be documented.

Don't worry about it being merged prematurely, it is blocked by the dissenting reviews, and I would not merge it without agreement from the community regardless.

[njr-11]

I updated the commit under this pull to align with the outcome of the vote (on the mailing list) which concluded with Option 2 being chosen, which is to state that virtual=true on Java SE 17 will behave like virtual=false and result in platform threads.

[aubi]

I'm fine with this. Thanks!

[KyleAure]

Looks good to me 👍🏼

[OndroMih]

Looks good.