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

Don't allow requests into restarting application #45670

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

stuartwdouglas
Copy link
Member

@stuartwdouglas stuartwdouglas commented Jan 17, 2025

If the app is restarting then we should not short-circuit the hot reload handler/scan lock logic.

This is not perfect, as there will always be a possible race, but makes it much less likely a request will hit a torn down app.

fixes #29646

This comment was marked as resolved.

@stuartwdouglas stuartwdouglas changed the title fix: don't allow requests into restarting application Don't allow requests into restarting application Jan 17, 2025
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/app-restarts branch from 830dbae to 4d27759 Compare January 17, 2025 04:15
@geoand
Copy link
Contributor

geoand commented Jan 17, 2025

I assume this is meant to close #29646?

@stuartwdouglas
Copy link
Member Author

I did not realize there was an existing issue, but yes, it seems like it.

@geoand
Copy link
Contributor

geoand commented Jan 17, 2025

🙏🏽

@geoand geoand added triage/waiting-for-ci Ready to merge when CI successfully finishes triage/backport labels Jan 17, 2025

This comment has been minimized.

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jan 17, 2025

The test failure should be fixed by #45681

If the app is restarting then we should not short-circuit the hot reload handler/scan lock logic.

This is not perfect, as there will always be a possible race, but makes it much less likely a request will hit a torn down app.
@geoand geoand force-pushed the stuartwdouglas/app-restarts branch from 4d27759 to 4ba3a46 Compare January 18, 2025 06:16
Copy link

quarkus-bot bot commented Jan 18, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 4ba3a46.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

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

io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 - History

  • io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor\#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7 at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:124) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856) - java.lang.RuntimeException
java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7
	at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:124)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)

📦 integration-tests/smallrye-context-propagation

io.quarkus.context.test.SimpleContextPropagationTest.testArcTCContextPropagationDisabled - History

  • expected: <2> but was: <3> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: expected: <2> but was: <3>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
	at io.quarkus.context.test.SimpleContextPropagationTest.testArcTCContextPropagationDisabled(SimpleContextPropagationTest.java:92)

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-6","-7","-8","-10","-11","-12","-13","-14"] to start with: ["-6", "-7", "-8", "-9"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-6","-7","-8","-10","-11","-12","-13","-14"]
to start with:
  ["-6", "-7", "-8", "-9"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:41)

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/websockets-next/deployment

io.quarkus.websockets.next.test.broadcast.BroadcastOnOpenTest.testLo - History

  • Messages: [c2:client1] ==> expected: <true> but was: <false> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: Messages: [c2:client1] ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at io.quarkus.websockets.next.test.broadcast.BroadcastOnOpenTest.assertBroadcast(BroadcastOnOpenTest.java:105)
	at io.quarkus.websockets.next.test.broadcast.BroadcastOnOpenTest.testLo(BroadcastOnOpenTest.java:45)

@geoand geoand merged commit fc7985e into quarkusio:main Jan 18, 2025
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 18, 2025
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jan 18, 2025
@gsmet gsmet modified the milestones: 3.19 - main, 3.18.0 Jan 21, 2025
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.

Exceptions on dev mode reload (with multiple inbound requests?)
3 participants