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

fixes #511 -- Support Exposing JVM Debug Port #512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ehigham
Copy link

@ehigham ehigham commented Mar 8, 2023

When users expose JVM debug ports, the JVM prints information about the debug port on initialization (ie before main is invoked). Discard stdout noise until the port is successfully read.

@@ -1075,6 +1075,14 @@ def testJavaopts(self):
self.gateway = JavaGateway.launch_gateway(javaopts=["-Xmx64m"])
self.assertTrue(self.gateway.jvm)

def testJvmDebugPort(self):
from random import randint
dport = randint(5000, 5999)
Copy link
Author

Choose a reason for hiding this comment

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

Using a random int in case tests are run in parallel and we run into port conflicts. I don't think the actual number matters.

Comment on lines +339 to +343
try:
_port = int(proc.stdout.readline())
except ValueError:
# If a JVM debug port is exposed, its details will be printed first
pass
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if there's a better way of doing this other than consuming just enough stdout to successfully parse the int.

@HyukjinKwon
Copy link
Member

Sorry for the delay. Now the tests are fixed per #519. Would you mind rebasing this one please?

When users expose JVM debug ports, the JVM prints information about
the debug port on initialization (ie before `main` is invoked).
Discard stdout noise until the port is successfully read.
@ehigham
Copy link
Author

ehigham commented Apr 23, 2024

Sorry for the delay. Now the tests are fixed per #519. Would you mind rebasing this one please?

No worries! Thanks for taking a look. Rebase pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants