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

Warn about usage of Quarkus#blockingExit on the main thread #28939

Merged
merged 2 commits into from Nov 1, 2022
Merged

Warn about usage of Quarkus#blockingExit on the main thread #28939

merged 2 commits into from Nov 1, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Oct 31, 2022

Relates to: #28899

@geoand geoand mentioned this pull request Oct 31, 2022
Closed
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 31, 2022
@quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Ensure that Quarkus.blockingExit is not called on main thread for tests
2131546 
StartupActionImpl#runMainClassBlocking which is used for
@QuarkusMainTest was incorrectly called on the main
thread before this change.
@geoand
Warn about usage of Quarkus#blockingExit on the main thread
04652b7 
Relates to: quarkusio#28899
@geoand
Copy link
Contributor Author

geoand commented Oct 31, 2022

I had to add another test to fix how the exit method was being called for tests. @stuartwdouglas you might want to take a look

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 31, 2022

Failing Jobs - Building 04652b7

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✖ JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✖ JVM Tests - JDK 17 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17 MacOS M1
✔️ JVM Tests - JDK 18
✔️ Maven Tests - JDK 11
⌛ Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/vertx/deployment 
! Skipped: extensions/agroal/deployment extensions/amazon-lambda-http/deployment extensions/amazon-lambda-rest/deployment and 340 more

📦 extensions/vertx/deployment

✖ io.quarkus.vertx.DuplicatedContextTest.testThatBlockingEventConsumersAreCalledOnDuplicatedContext - More details - Source on GitHub

(RECIPIENT_FAILURE,8185) io.smallrye.mutiny.TimeoutException
	at io.vertx.core.eventbus.Message.fail(Message.java:141)
	at io.quarkus.vertx.runtime.VertxRecorder$3$1$1.handle(VertxRecorder.java:122)

⚙️ JVM Tests - JDK 17 #

- Failing: devtools/cli

📦 devtools/cli

✖ io.quarkus.cli.CliProjectJBangTest.testCreateAppDefaults line 64 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 
Expected OK return code. Result:
result: {

✖ io.quarkus.cli.CliProjectJBangTest.testCreateCliDefaults line 127 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 
Expected OK return code. Result:
result: {

✖ io.quarkus.cli.CliProjectJBangTest.testCreateAppOverrides line 98 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 
Expected OK return code. Result:
result: {

stuartwdouglas
stuartwdouglas approved these changes Nov 1, 2022
View reviewed changes
core/runtime/src/main/java/io/quarkus/runtime/Quarkus.java
@@ -182,6 +183,12 @@ public static void waitForExit() {
* Must not be called by the main thread, or a deadlock will result.
*/
public static void blockingExit() {
if (Thread.currentThread().getThreadGroup().getName().equals("main") &&
Copy link
Member

Choose a reason for hiding this comment

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

I think this will be ok. If we get reports of false positives though we should probably move it to some kind of ThreadLocal marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely

@geoand geoand merged commit 24a754e into quarkusio:main Nov 1, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 1, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 1, 2022
@geoand geoand deleted the #28899 branch November 1, 2022 06:23
@gsmet gsmet modified the milestones: 2.15 - main, 2.14.0.Final Nov 1, 2022
@gsmet gsmet modified the milestones: 2.14.0.Final, 2.13.4.Final Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

@stuartwdouglas stuartwdouglas stuartwdouglas approved these changes

Assignees
No one assigned
Labels
area/core triage/flaky-test
Projects
None yet
Milestone
2.13.4.Final
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@geoand @aloubyansky @stuartwdouglas @gsmet