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.

Sign up for GitHub

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

Jump to bottom

SmallRye Context Propagation 1.0.16 #11410

Merged
merged 5 commits into from Oct 28, 2020
Merged

SmallRye Context Propagation 1.0.16 #11410

merged 5 commits into from Oct 28, 2020

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Aug 17, 2020

No description provided.

@FroMage FroMage requested a review from manovotn August 17, 2020 08:36
@boring-cyborg boring-cyborg bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/smallrye labels Aug 17, 2020
@FroMage FroMage mentioned this pull request Aug 17, 2020
Closed
@FroMage
Copy link
Member Author

FroMage commented Aug 17, 2020

Fixes #11378 as well.

@FroMage
Copy link
Member Author

FroMage commented Aug 17, 2020

@manovotn turns out the CI for quarkus-arc-deployment fails with:

java.lang.NoSuchMethodError: io.quarkus.arc.test.unproxyable.SynthProxiableBeanWithoutNoArgConstructorTest$SynthBean.<init>(Ljava/lang/String;)V
	at io.quarkus.arc.test.unproxyable.SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_Bean.create(SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_Bean.zig:139)
	at io.quarkus.arc.test.unproxyable.SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_Bean.create(SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_Bean.zig:154)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:96)
	at io.quarkus.arc.impl.AbstractSharedContext.access$000(AbstractSharedContext.java:14)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:29)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:26)
	at io.quarkus.arc.impl.LazyValue.get(LazyValue.java:26)
	at io.quarkus.arc.impl.ComputingCache.computeIfAbsent(ComputingCache.java:69)
	at io.quarkus.arc.impl.AbstractSharedContext.get(AbstractSharedContext.java:26)
	at io.quarkus.arc.test.unproxyable.SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_ClientProxy.arc$delegate(SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_ClientProxy.zig:110)
	at io.quarkus.arc.test.unproxyable.SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_ClientProxy.getString(SynthProxiableBeanWithoutNoArgConstructorTest_SynthBean_d2489e99512ed6afb3250b515819124dd8be8184_Synthetic_ClientProxy.zig:229)
	at io.quarkus.arc.test.unproxyable.SynthProxiableBeanWithoutNoArgConstructorTest.testSyntheticBean(SynthProxiableBeanWithoutNoArgConstructorTest.java:70)

@FroMage
Copy link
Member Author

FroMage commented Aug 17, 2020

It also fails on my machine locally.

@manovotn
Copy link
Contributor

@FroMage my bad, didn't push my local changes - the test bean class needs to be static.

@FroMage
Copy link
Member Author

FroMage commented Aug 17, 2020

Haha, great, thanks, no worries :)

@manovotn
Copy link
Contributor

There seems to be some more test fails, but those shouldn't be because of my change. It is linkage error from JDK 9 wrapper for completable future...

@FroMage
Copy link
Member Author

FroMage commented Aug 17, 2020

2020-08-17T14:12:30.2312122Z java.lang.LinkageError: loader constraint violation: when resolving method 'void io.smallrye.context.Jdk9CompletableFutureWrapper.(io.smallrye.context.SmallRyeThreadContext, java.util.concurrent.CompletableFuture, java.util.concurrent.Executor, boolean)' the class loader io.quarkus.bootstrap.classloading.QuarkusClassLoader @76ac879c of the current class, io/smallrye/context/impl/JdkSpecificImpl, and the class loader 'app' for the method's defining class, io/smallrye/context/Jdk9CompletableFutureWrapper, have different Class objects for the type io/smallrye/context/SmallRyeThreadContext used in the signature (io.smallrye.context.impl.JdkSpecificImpl is in unnamed module of loader io.quarkus.bootstrap.classloading.QuarkusClassLoader @76ac879c, parent loader 'platform'; io.smallrye.context.Jdk9CompletableFutureWrapper is in unnamed module of loader 'app')

DAMNIT

@FroMage
Copy link
Member Author

FroMage commented Aug 17, 2020

@dmlloyd or @stuartwdouglas do you happen to know if there's anything obvious I should be looking at for errors like these? This is due to a newly MultiRelease jar with JDK9-specific classes.

@FroMage
Copy link
Member Author

FroMage commented Aug 17, 2020

@stuartwdouglas is it possible that QuarkusClassLoader doesn't know how to deal with MR jars?

@stuartwdouglas
Copy link
Member

I will look into it.

@boring-cyborg boring-cyborg bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven area/testing labels Aug 18, 2020
@stuartwdouglas
Copy link
Member

I have pushed a fix to this PR, lets see what CI says.

@FroMage
Copy link
Member Author

FroMage commented Aug 18, 2020

Thanks. Now failures in devtools :(

@FroMage
Copy link
Member Author

FroMage commented Aug 18, 2020

This works on my machine so let's restart CI.

@stuartwdouglas
Copy link
Member

I pushed a fix, there were some legitimate failures

@FroMage
Copy link
Member Author

FroMage commented Aug 18, 2020

Now it's the MP-CP TCK failing, which is a problem because we've added new methods in SR-CP that will end up in the spec, but we can't release the spec ATM, so we can't release a new version of the TCK which fixes those tests.

@FroMage
Copy link
Member Author

FroMage commented Aug 18, 2020

So one of the TCK failure is a real issue and I'll wait for smallrye/smallrye-context-propagation#195 to be merged so I can pull the new release.

The other two I should ignore until we can release a new TCK.

@kenfinnigan do you happen to know how I can ignore specific test methods from the TCK? I tried the obvious surefire config, but got:

Method filter prohibited in includes|excludes|includesFile|excludesFile parameter: ThreadContextTest#withContextCaptureDependentCompletableFuturesRunWithContext

@gsmet
Copy link
Member

gsmet commented Aug 18, 2020

@FroMage this is done like that in other MP TCKs: https://github.com/quarkusio/quarkus/blob/master/tcks/microprofile-rest-client/pom.xml#L34

@FroMage
Copy link
Member Author

FroMage commented Aug 18, 2020

This excludes an entire file, I can't do that I only need to exclude two tests in that class.

@gastaldi
Copy link
Contributor

gastaldi commented Oct 9, 2020

The Quarkus CI / MicroProfile TCKs Tests failed but I don't think it's related to these changes?

@Ladicek
Copy link
Contributor

Ladicek commented Oct 12, 2020

If I remember correctly, it is quite related :-) I have a preliminary PR to SmallRye Fault Tolerance that makes everything run on a single thread pool provided by an integrator, but I've uncovered some issues with how liberally we submit tasks to thread pools and wait for their completion. I'm working on a fix for that.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 19, 2020

OK, so I have reimplemented and optimized bulkheads in SmallRye Fault Tolerance (optimized as in: bulkheads no longer submit any tasks to an executor). It's all built on top of the changes I did to implement MicroProfile Fault Tolerance 3.0, so wouldn't be exactly easy to backport to current master. But MP FT 3.0 should be released "soon" so I hope that's fine.

@FroMage
Copy link
Member Author

FroMage commented Oct 21, 2020

I'll make a new SR-CP release and rebase this branch ASAP.

@FroMage
Copy link
Member Author

FroMage commented Oct 21, 2020

Upgraded to latest SR-CP version, rebased on master, fixed conflicts. I have to write some docs about the new features.

@FroMage
Copy link
Member Author

FroMage commented Oct 21, 2020

Failed with:

2020-10-21T14:34:50.1749165Z 	[error]: Build step io.quarkus.arc.deployment.ArcProcessor#registerBeans threw an exception: javax.enterprise.inject.spi.DefinitionException: Interceptor has no bindings: io.smallrye.context.impl.cdi.SmallRyeCurrentThreadContextInterceptor
2020-10-21T14:34:50.1762017Z 	at io.quarkus.arc.processor.Interceptors.createInterceptor(Interceptors.java:44)
2020-10-21T14:34:50.1766515Z 	at io.quarkus.arc.processor.BeanDeployment.findInterceptors(BeanDeployment.java:1052)
2020-10-21T14:34:50.1776096Z 	at io.quarkus.arc.processor.BeanDeployment.registerBeans(BeanDeployment.java:220)
2020-10-21T14:34:50.1786451Z 	at io.quarkus.arc.processor.BeanProcessor.registerBeans(BeanProcessor.java:115)
2020-10-21T14:34:50.1795862Z 	at io.quarkus.arc.deployment.ArcProcessor.registerBeans(ArcProcessor.java:381)
2020-10-21T14:34:50.1803462Z 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2020-10-21T14:34:50.1810664Z 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2020-10-21T14:34:50.1874797Z 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2020-10-21T14:34:50.1877039Z 	at java.lang.reflect.Method.invoke(Method.java:498)
2020-10-21T14:34:50.1878316Z 	at io.quarkus.deployment.ExtensionLoader$2.execute(ExtensionLoader.java:936)
2020-10-21T14:34:50.1879323Z 	at io.quarkus.builder.BuildContext.run(BuildContext.java:277)
2020-10-21T14:34:50.1881055Z 	at org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
2020-10-21T14:34:50.1882920Z 	at org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:2046)
2020-10-21T14:34:50.1884456Z 	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1578)
2020-10-21T14:34:50.1885895Z 	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1452)
2020-10-21T14:34:50.1886845Z 	at java.lang.Thread.run(Thread.java:748)
2020-10-21T14:34:50.1887518Z 	at org.jboss.threads.JBossThread.run(JBossThread.java:479)

So I must have forgotten something.

@manovotn
Copy link
Contributor

manovotn commented Oct 21, 2020 •
edited

As per our Zulip conversation, the annotation is not indexed. This is likely due to missing beans.xml in the api archive of SR CP.

FroMage and others added 4 commits October 21, 2020 17:22
@FroMage
Upgrade to SmallRye-ContextPropagation 1.0.19
97305e9 
Can now inject SmallRye variants which have more features
@manovotn @FroMage
Automagically add no-args constructor to synthetic beans.
efb657e
@FroMage @manovotn
Update extensions/arc/deployment/src/test/java/io/quarkus/arc/test/un…
0059ed9 
…proxyable/SynthProxiableBeanWithoutNoArgConstructorTest.java

Co-authored-by: Matej Novotny <matej.novotny2@gmail.com>
@stuartwdouglas @FroMage
Multi release jar ClassLoader fixes
ef92b49
@FroMage
Copy link
Member Author

FroMage commented Oct 21, 2020

Should be fixed by a new release. Rebased again.

@FroMage
Copy link
Member Author

FroMage commented Oct 22, 2020

Apparently the FT TCK got killed. Not sure if it hanged or CI just ran out of patience.

@gsmet
Copy link
Member

gsmet commented Oct 22, 2020

@FroMage it usually only takes ~40 minutes so there's something fishy if it got cancelled after 2 hours.

Looks like the same issue we had previously.

@FroMage
Copy link
Member Author

FroMage commented Oct 22, 2020

@gsmet thanks a lot, you're right.

So @Ladicek we need to couple this with a new SR-FT release?

@Ladicek
Copy link
Contributor

Ladicek commented Oct 22, 2020

@FroMage That would be best, yea. I'm not sure when MP FT 3.0 is released, and I have all the changes on my next branch which is for MP FT 3.0 :-/ I can try to backport the thread pool changes to master, if you want.

@FroMage
Copy link
Member Author

FroMage commented Oct 27, 2020

I'm going to make a new SR-CP release with support for Java 12 anyway, but are we talking days or months?

@Ladicek
Copy link
Contributor

Ladicek commented Oct 27, 2020

I asked @radcortez and it seems it's not "days". I'll grab a bottle and do some backporting -- sorry for stalling this so long.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 27, 2020

I've added a workaround as discussed on Zulip, and released SRye FT 4.3.2: https://repo.maven.apache.org/maven2/io/smallrye/smallrye-fault-tolerance/4.3.2/ You can bump in this PR, or I can submit a separate PR, but I guess doing it here would be faster. There's no other change required, just bumping the version number in the BOM.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 27, 2020

Actually I couldn't even bump SRye FT to 4.3.2 in isolation, because it uses the new method from SRye ConProp. So it needs to be udpated in this PR anyway :-)

@FroMage
Copy link
Member Author

FroMage commented Oct 27, 2020

OK thanks a lot. Bumped it, let's see what CI says.

@FroMage
Copy link
Member Author

FroMage commented Oct 28, 2020

CI failed with IO issue in docker pull. Restarting.

@FroMage
Copy link
Member Author

FroMage commented Oct 28, 2020

TCK CI passed, so that's good news.

@FroMage FroMage merged commit a0d85f2 into quarkusio:master Oct 28, 2020
@FroMage
Copy link
Member Author

FroMage commented Oct 28, 2020

WOOHOO!

@FroMage FroMage added this to the 1.10 - master milestone Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi approved these changes

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven area/smallrye area/testing
Projects
None yet
Milestone
1.10.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@FroMage @manovotn @stuartwdouglas @gsmet @kenfinnigan @Ladicek @dmlloyd @mkouba @gastaldi