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

WFLY-15687 Add support for CDI 4 Build compatible extensions #15474

Merged
merged 2 commits into from
May 4, 2022

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented Apr 26, 2022

This PR superseeds #15464. I have squashed Scott's commits into one and added mine on top.

JIRA link - https://issues.redhat.com/browse/WFLY-15687

There are still some remaining TCK failures, namely 12 of them.

[ERROR] Tests run: 1851, Failures: 12, Errors: 0, Skipped: 74

10 of these are JSF related (old artifact using now removed deprecated APIs) - fixed by updating JSF impl artifact.

The remaining two are what concerns us:

  • SyntheticBeanWithLookupTest- this one is (most likely) in WFLY's integration code but I am failing to see how to properly fix that. The last commit in this PR adds a very nasty hack which makes the test pass, but I am anything but happy with it :-/
  • StartupShutdownTest - this one needs to be fixed on Weld's side, see https://issues.redhat.com/browse/WELD-2717, there is already a pending PR

CC @starksm64

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Apr 26, 2022
@manovotn
Copy link
Contributor Author

BTW, with these changes and with WELD-2717, I am only seeing 10 TCK failures all of which are:

Caused by: java.lang.NoSuchMethodError: 'void jakarta.enterprise.inject.spi.BeforeBeanDiscovery.addAnnotatedType(jakarta.enterprise.inject.spi.AnnotatedType)'
	at com.sun.jsf-impl@3.0.0.SP04//com.sun.faces.flow.FlowDiscoveryCDIExtension.beforeBeanDiscovery(FlowDiscoveryCDIExtension.java:77)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.weld.core@5.0.1.SNAPSHOT//org.jboss.weld.injection.StaticMethodInjectionPoint.invoke(StaticMethodInjectionPoint.java:95)

@starksm64
Copy link
Contributor

@manovotn Ok, great, I'll see if can get our Mojarra fork updated.

@manovotn
Copy link
Contributor Author

@manovotn Ok, great, I'll see if can get our Mojarra fork updated.

@starksm64 That fork is long updated now. I first sent jboss/mojarra#90 which was forgotten and then later Brian did jboss/mojarra#92 which got merged.
But I think there was no release since.

@jasondlee
Copy link
Contributor

I'm working on making a release of jsf-impl now. There's some weirdness with the versions I need to figure out, but I'm actively working on this.

@manovotn
Copy link
Contributor Author

I've spent some "quality" tie debugging the problem with SyntheticBeanWithLookupTest and I think I've found a way to workaround this in a cleaner fashion directly in Weld.
Following JIRA captures it along with PR - https://issues.redhat.com/browse/WELD-2718

Therefore, I'll remove the hacky approach from this PR and we can probably turn the draft into actual PR.
Once Weld 5 Final is out, WFLY can update and with that should have a passing CDI TCK.

@manovotn manovotn force-pushed the weldLiteExtensionTranslator branch from 1e20bf3 to b341bb9 Compare April 27, 2022 15:03
@manovotn manovotn marked this pull request as ready for review April 27, 2022 15:06
@jasondlee
Copy link
Contributor

The JSF update now has a PR: #15487

@manovotn
Copy link
Contributor Author

Nice @jasondlee, thanks!
Once we merge both PRs, I could bring back WFLY testing on Weld PRs (if there is a nightly WFP build).

@jasondlee
Copy link
Contributor

That would be great. :)

@bstansberry
Copy link
Contributor

@manovotn The JSF upgrade is merged. We produce a WFP wildfly-preview-latest-SNAPSHOT.zip nightly from the https://ci.wildfly.org/viewType.html?buildTypeId=WF_WildflyPreviewNightly job.

@bstansberry
Copy link
Contributor

/retest

Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manovotn manovotn force-pushed the weldLiteExtensionTranslator branch from b341bb9 to 51add60 Compare April 29, 2022 07:42
@manovotn
Copy link
Contributor Author

@manovotn
Copy link
Contributor Author

I have also released Weld 5 Final today, so I'll update this PR with it

@bstansberry
Copy link
Contributor

bstansberry commented Apr 29, 2022

@manovotn @kabir The WildFly Preview job failed with a java.lang.VerifyError in the MP Reactive Messaging TCK. That job uses SE 17 to run tests. I'll kick off a custom run of the night WFP SE 11 and SE 17 jobs using this branch to see what happens.

https://ci.wildfly.org/viewLog.html?buildId=303213&tab=buildResultsDiv&buildTypeId=WF_WildFlyPreviewLinuxJdk11

https://ci.wildfly.org/viewLog.html?buildId=303215&tab=buildResultsDiv&buildTypeId=WF_WildFlyPreviewLinuxJdk17

@bstansberry
Copy link
Contributor

Those jobs failed as well, including with SE 17.

The dependencyManagement we use when running the MP 5 TCKs with WFP is out of date though, so I'm going to get that cleaned up and then I'll see if that makes the problem go away.

@bstansberry bstansberry mentioned this pull request Apr 30, 2022
@manovotn
Copy link
Contributor Author

manovotn commented May 2, 2022

@manovotn @kabir The WildFly Preview job failed with a java.lang.VerifyError in the MP Reactive Messaging TCK

I am afraid I have no idea why. I am not aware of any change in Weld between CR2 and Final that could cause it. Besides, if it were in Weld, I assume we'd see failures sooner and/or all over the place.
Back when I sent the PR for Weld 5 integration, these TCKs passed - did the job run on JDK 17 at the time? Or did the TCK config change recently?

@bstansberry
Copy link
Contributor

@manovotn I'm confused as well. I thought it might be due to WFLY-16337 (i.e. the changes here surfaced an existing problem) but a test branch combining this and a fix for that had the same issue:

#15516

I'll discuss with @kabir as he's our RSO expert. AIUI these failures are on the client side. The client-side classpath is tricky.

We've been running PRs against WFP+SE17 for a long time, plus nightlies. So something is odd here.

@scottmarlow
Copy link
Contributor

scottmarlow commented May 2, 2022

VerifyError

https://www.eclipse.org/lists/cdi-dev/msg00586.html is reporting the same VerifyError with GlassFish tests

Caused by: java.lang.VerifyError: Expecting a stackmap frame at branch target 12
Exception Details:
Location:
org/glassfish/jms/injection/RequestedJMSContextManager$Proxy$_$$_WeldClientProxy.toString()Ljava/lang/String; @4: ifne
Reason:
Expected stackmap frame at this location.
Bytecode:
0000000: 2ab4 0018 9a00 082a b700 5bb0 b800 2e4c
0000010: 2ab4 0034 b600 3ac0 001c 59b6 005c 2b59
0000020: c600 0cc0 0030 b600 3da7 0004 57a7 0013
0000030: 2b59 c600 0cc0 0030 b600 3da7 0004 57bf
0000040: 5aa5 0004 b02a c000 28b0
Exception Handler Table:
bci [16, 48] => handler: 48

@bstansberry
Copy link
Contributor

@manovotn I've been trying simple component upgrades (i.e. not the rest of this PR) and this problem appears with this commit:

weld/core@1f0b5c6

To reproduce in ee-9/pom.xml I update the CDI API to 4.0.0, weld-api to 5.0.CR2, and weld-core to a SNAPSHOT built from that sha. Build the server and then

mvn install -Dts.ee9 -pl testsuite/integration/microprofile-tck/reactive-streams-operators

If I use weld 5.0.0.CR2 instead of that snapshot the problem doesn't appear.

@manovotn
Copy link
Contributor Author

manovotn commented May 2, 2022

@bstansberry the commit you linked is a POM change in Weld's SE artifacts and those aren't used in WFLY at all from what I know. Also performing a build with a revert on this commit doesn't solve anything.

If I were to guess, you meant weld/core@18af30b?
That one seems to be the turning point; if I revert that, I get the TCK to pass.

That's weird, the issue was linked primarily to self interception and if I am not mistaken, the class being proxied is this one and that seems just fine. At least we know what's causing it, but I am still trying to grasp why.

@starksm64
Copy link
Contributor

starksm64 commented May 2, 2022 via email

@bstansberry
Copy link
Contributor

Sorry for the bad link @manovotn :(

Yes, weld/core@18af30b is the one I meant

@manovotn
Copy link
Contributor Author

manovotn commented May 3, 2022

So, the weld/core@18af30b is definitely the cause.
It seems to happen when there is a hierarchy of at least two classes, where the super class is in a different package and has protected methods which are then subject to proxying by Weld.

The error, however, is completely off because those proxy files don't even have a stackmap in the first place! (you can see that if you inspect them with javap -v -p)
jboss-classfilewriter apparently creates version 50.0 files (JDK 6) which can, by JLS definition, lack the stackmap in which case JVM does type inference. So the VerifyError is actually just misleading and the cause if likely somewhere else. This is also confirmed by the fact that the error points to toString method which hasn't changed in any way and the generated bytecode for this method is identical between CR2 and Final.
That said, I was unable to determine the actual cause so far.

All I know is that from the offending commit, the change is that protected methods on proxy are now invoked via invokevirtual/invokeinterface instead of via method handler invocation (reflection). This should, in theory, be valid but instead blows up :-/

@bstansberry
Copy link
Contributor

In case it's useful, ReactiveStreamsCdiTck is a javax.* namespace class, which means it's getting bytecode transformed when installed into WFP.

@kabir
Copy link
Contributor

kabir commented May 4, 2022

It might be interesting to see the difference in bytecode between the original and transformed class. @ropalka (I think you worked on this) - is there a hook for @manovotn to be able to dump the transformed class bytes?

@manovotn
Copy link
Contributor Author

manovotn commented May 4, 2022

@kabir I managed to create a reproducer yesterday and have bytecode (weld can dump all created proxies via an option).
Still didn't help though; I suspect that there is a catch somewhere in classfilewriter in combination with bytecode verification but it is more of a guess than anything else.

I will roll back the problematic commit and issue a new Weld release.

@manovotn manovotn force-pushed the weldLiteExtensionTranslator branch from 3d7edb1 to 54b2954 Compare May 4, 2022 11:56
@manovotn manovotn force-pushed the weldLiteExtensionTranslator branch from 54b2954 to dc2c99d Compare May 4, 2022 12:00
@manovotn
Copy link
Contributor Author

manovotn commented May 4, 2022

@bstansberry this PR is rebased onto main to resolve the conflict and has been updated to Weld core 5.0.0.SP1 which should fix the problem CI was showing before.

@bstansberry
Copy link
Contributor

Thanks @manovotn!

FYI, the Windows - JDK 11 job is going to show some weird failures that we're investigating and that have popped up elsewhere, so not related to this PR. So don't worry about them. I kicked off a re-run of that job.

@bstansberry bstansberry merged commit 1aa3f11 into wildfly:main May 4, 2022
@bstansberry
Copy link
Contributor

Thanks, @manovotn & @starksm64 !

@manovotn manovotn deleted the weldLiteExtensionTranslator branch May 4, 2022 22:00
@starksm64
Copy link
Contributor

starksm64 commented Oct 11, 2022 via email

@manovotn
Copy link
Contributor Author

@starksm64 are you sure this comment belongs to this 5 months old issue? I have no clue what errors are you talking about here :)

@starksm64
Copy link
Contributor

I did not make that comment 8 hours ago, so where did it come from? It seems like it would have been a comment made on May 5 where I then commented about the bytecode in the stack trace. It certainly is not relevant now.

@manovotn
Copy link
Contributor Author

I did not make that comment 8 hours ago,

Yea, that's what confused me. Probably a glitch in the Matrix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
6 participants