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

Use sun.misc.IOUtils new API readAllBytes() #8312

Merged
merged 1 commit into from
Jan 15, 2020

Conversation

JasonFengJ9
Copy link
Member

@JasonFengJ9 JasonFengJ9 commented Jan 15, 2020

Use sun.misc.IOUtils new API readAllBytes()

Replace IOUtils.readFully(is, -1, true) with IOUtils.readAllBytes(is) to avoid IOException: length cannot be negative: -1.

[ci skip] because the new API readAllBytes(is) is not available at extension repo openj9 branch.

Notes:

  1. Though readNBytes(is, Integer.MAX_VALUE) has same result, readAllBytes(is) seems better choice;
  2. API doc within sun.misc.IOUtils specifies @since 1.9 or @since 11 though this is a dedicated Java 8 extension rep https://github.com/ibmruntimes/openj9-openjdk-jdk8/blob/e6095d9adfe3c072d483a7df3fcdf1eeec76044b/jdk/src/share/classes/sun/misc/IOUtils.java#L284-L302
    @andrew-m-leonard any insights?

Reviewer: @DanHeidinga

Fixes #8308
Fixes #8309

Signed-off-by: Jason Feng fengj@ca.ibm.com

Replace IOUtils.readFully(is, -1, true) with IOUtils.readAllBytes(is) to
avoid "IOException: length cannot be negative: -1".

[ci skip] because the new API is not available at extension repo openj9
branch.

Signed-off-by: Jason Feng <fengj@ca.ibm.com>
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

lgtm

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk8

@DanHeidinga
Copy link
Member

@JasonFengJ9 Can you port the change to the 0.18 branch?

@DanHeidinga
Copy link
Member

FYI - I'm not sure the PR build will pass as it depends on the openjdk import

@DanHeidinga
Copy link
Member

2. API doc within sun.misc.IOUtils specifies @since 1.9 or @since 11 though this is a dedicated Java 8 extension rep

This is due to the changes being a backport. I wouldn't worry about incorrect since tags in the upstream code

@pshipton
Copy link
Member

Started testing with the openj9-staging branch here https://ci.eclipse.org/openj9/view/Pipelines/job/Pipeline-Release-Build/144/

@JasonFengJ9
Copy link
Member Author

@DanHeidinga created #8315 for 0.18 branch.

@JasonFengJ9
Copy link
Member Author

This PR made change to https://github.com/eclipse/openj9/blob/master/jcl/src/java.base/share/classes/java/lang/invoke/SecurityFrameInjector.java
@pshipton the component label is comp:vm instead of comp:test, is that correct?

@pshipton
Copy link
Member

@JasonFengJ9
Copy link
Member Author

raw = IOUtils.readFully(stream, 1024, false);

This works as before because of the back compatibility implemented by
https://github.com/ibmruntimes/openj9-openjdk-jdk8/blob/e6095d9adfe3c072d483a7df3fcdf1eeec76044b/jdk/src/share/classes/sun/misc/IOUtils.java#L284-L312

@JasonFengJ9
Copy link
Member Author

[ERR] Caused by: java.io.IOException: length cannot be negative: -1
 [ERR] 	at sun.misc.IOUtils.readFully(IOUtils.java:305)
 [ERR] 	at java.lang.invoke.SecurityFrameInjector$1.run(SecurityFrameInjector.java:100)
 [ERR] 	... 10 more

This seems same error.

@JasonFengJ9
Copy link
Member Author

JasonFengJ9 commented Jan 15, 2020

https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_extended.functional_s390x_linux_OpenJDK8/1/

This appears the initial build triggered by JDK8 Acceptance pipeline.
There is no failure in https://ci.eclipse.org/openj9/job/Pipeline_Build_Test_JDK8_s390x_linux/1274/ however it has only sanity.functional.

@pshipton could you launch another extended.functional test build based on this PR?

@pshipton
Copy link
Member

pshipton commented Jan 15, 2020

@pshipton pshipton self-assigned this Jan 15, 2020
@pshipton pshipton merged commit 9a48d30 into eclipse-openj9:master Jan 15, 2020
@pshipton
Copy link
Member

pshipton commented Jan 15, 2020

Although I merged this, I expect it has broken jdk14 and jdk(next). Hopefully the readAllBytes() API will be delivered there shortly, otherwise we'll have to use the processor to handle these streams differently.

Nm, I forgot this was specific to jdk8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment