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

Do not check if debug port can be used in DEV mode on restart #28620

Conversation

michalvavrik
Copy link
Contributor

@michalvavrik michalvavrik commented Oct 15, 2022

fixes: #28187

Sometimes when you add dependency in DEV mode, app is restarting, the runner is shut down by the hook and firstStart is called, error is logged saying that port is in use; after restart, JVM won't listen for a socket connection on the port after restart. This happens because at the time we check if there is some service listening on the IP/port, the previous runner is still up. Under given circumstances, the order of things is as follow:

  • trigger restart
  • prepare new DEV mode launcher (where socket check takes place)
  • close previous runner (vacate IP/port)
  • start listening for transport dt_socket at address xy

and we know the port is going to be available. There is already a comment in the code that is saying we should not check the port on restart, and that's what this PR does. I didn't manage to reproduce the issue with Gradle (it seemed to me like changes in build.gradle are not reflected at all, thus it could not happen).

I know there is a draft #28256 from @sberyozkin, but I want respectfully propose this as I think waiting isn't right strategy (please see the order of things mentioned above).

@quarkus-bot quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven labels Oct 15, 2022
@quarkus-bot

This comment has been minimized.

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

@michalvavrik Thanks, it really makes sense, given your explanation, can you please confirm that a breakpoint is actually activated after a restart and some invocation on the demo endpoint ?

@sberyozkin
Copy link
Member

@michalvavrik By the way, please check the test, it uses test containers, I don't think the failure is related but just in case...

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Oct 16, 2022

@sberyozkin sure, I was testing it with IDE attached to the process and I run combinations (attached -> add dep -> attach again; not attached -> add dep -> attach; ..), though I realize I can't cover all scenarios and some IT would be nicer. I run the test and it works (I'd not expect it to fail as I don't think it has anything to do with DEV mojo where the changes are).

@michalvavrik michalvavrik force-pushed the feature/fix-port-listen-after-dev-restart branch from 9a262ba to fbd35b3 Compare October 16, 2022 10:54
@michalvavrik
Copy link
Contributor Author

btw. I mentioned constructor that I added had too high access modifier, so I pushed change.

@sberyozkin
Copy link
Member

Sounds good

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 16, 2022

Failing Jobs - Building fbd35b3

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

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/flyway/deployment 
! Skipped: integration-tests/flyway integration-tests/hibernate-orm-tenancy/datasource integration-tests/hibernate-orm-tenancy/schema and 2 more

📦 extensions/flyway/deployment

io.quarkus.flyway.test.FlywayExtensionInitSqlTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:689)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

@sberyozkin
Copy link
Member

Flyway test is currently flaky, so lets merge

@sberyozkin sberyozkin merged commit 24ba1db into quarkusio:main Oct 16, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Oct 16, 2022
@michalvavrik michalvavrik deleted the feature/fix-port-listen-after-dev-restart branch October 16, 2022 17:05
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.3.Final Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven kind/bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5005 port is not freed in devmode during the restart
3 participants