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

Add loom pinning tests #8289

Merged
merged 3 commits into from
Mar 17, 2024
Merged

Add loom pinning tests #8289

merged 3 commits into from
Mar 17, 2024

Conversation

yschimke
Copy link
Collaborator

No description provided.

@yschimke yschimke added the loom Relates to Loom JDK support label Mar 16, 2024
@yschimke yschimke mentioned this pull request Mar 16, 2024
Open
yschimke
yschimke commented Mar 16, 2024
View reviewed changes
build.gradle.kts
@@ -169,8 +169,7 @@ subprojects {

if (platform == "loom") {
jvmArgs(
"-Djdk.tracePinnedThread=full",
"--enable-preview"
"-Djdk.tracePinnedThreads=short",
Copy link
Collaborator Author

@yschimke yschimke Mar 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

note: the s was missing.

@yschimke
Copy link
Collaborator Author 

    okhttp3.internal.http2.Http2Writer.flush(Http2Writer.kt:125) <== monitors:1
    okhttp3.internal.http2.Http2Connection.newStream(Http2Connection.kt:271) <== monitors:2
Thread[#62,ForkJoinPool-1-worker-3,5,CarrierThreads]
    okhttp3.internal.http2.Http2Stream$FramingSource.read(Http2Stream.kt:395) <== monitors:1

@yschimke
Copy link
Collaborator Author

@swankjesse Going to land even though it has a failing test for loom. The break is real, and I'd like to fix.

requesting post review, tests and CI only.

@yschimke yschimke merged commit 9000e32 into square:master Mar 17, 2024
19 of 20 checks passed
swankjesse
swankjesse reviewed Apr 4, 2024
View reviewed changes
Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

LGTM

container-tests/src/test/java/okhttp3/containers/BasicLoomTest.kt
val platform = PlatformRule()

@Container
val mockServer: MockServerContainer = MockServerContainer(MOCKSERVER_IMAGE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why isn’t this using MockWebServer?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll put a comment. I only wanted to fail on the client side. Not to test our mockwebserver is loom compliant also.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. I’d like to also fix MockWebServer to be compliant!

container-tests/src/test/java/okhttp3/containers/BasicLoomTest.kt

executor = newVirtualThreadPerTaskExecutor()

System.setOut(PrintStream(capturedOut))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What’s this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have a programmatic way to detect loom blocking. Just capture this warning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha.

This was referenced Apr 6, 2024
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swankjesse swankjesse swankjesse left review comments

Assignees
No one assigned
Labels
loom Relates to Loom JDK support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yschimke @swankjesse