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

Close the delayed log handler if exception #20797

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Oct 15, 2021

If the handler has not been activated, we flush the content of the log
when we have an exception. Otherwise, the user is missing important
information.

This is especially important when a test resource does not start properly.

@stuartwdouglas could you have a look at this one? I saw a user complain about this and I also had the issue lately in the quickstarts and this helped. I'm not sure if I have found all the potential places I should adjust. Any other you have in mind?

@@ -194,6 +195,9 @@ public void close() throws Throwable {

return state;
} catch (Throwable e) {
if (!InitialConfigurator.DELAYED_HANDLER.isActivated()) {
InitialConfigurator.DELAYED_HANDLER.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't activate make more sense here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, activate() is private so no :).

And it won't work. If the handler has not been activated, we don't have any nested handler contributed by the Quarkus bootstrap so we somehow have to push the log records ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, was thinking of something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remembered: LoggingSetupRecorder.handleFailedStart(); is what I was after

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should register a shutdown hook instead in io.quarkus.bootstrap.logging.InitialConfigurator? Also LoggingSetupRecorder.handleFailedStart(); is potentially better than just calling close, as it will mostly respect the logging settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

@geoand I think this is ready now. We tried the shutdown hook approach but it doesn't work (see #21702) so the approach in this PR is the one we will pursue.

I changed the PR to use activateLogging() as you suggested it.

@gsmet gsmet mentioned this pull request Nov 25, 2021
@gsmet gsmet force-pushed the close-delayed-handler branch from 5d19217 to 5f15374 Compare December 23, 2021 12:02
@@ -36,8 +38,6 @@
public static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace
.create("io.quarkus.test.main.jvm");

private static Map<String, String> devServicesProps;
Copy link
Member Author

Choose a reason for hiding this comment

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

This field was unused.

If the handler has not been activated, we flush the content of the log
when we have an exception. Otherwise, the user is missing important
information.

This is especially important when a resource does not start properly.
@gsmet gsmet force-pushed the close-delayed-handler branch from 5f15374 to 794c71b Compare December 23, 2021 12:04
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 23, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 23, 2021

Failing Jobs - Building 794c71b

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 2 more

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed

@gsmet gsmet merged commit f133a1b into quarkusio:main Dec 23, 2021
@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Dec 23, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants