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

Jetty 12 - use JVM provided null OutputStream #8789

Merged
merged 5 commits into from
Nov 9, 2022

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 31, 2022

Turns out we really only used our Null OutputStream and Null Writer from test cases.

So just use the JVM provided ones instead.

+ This also honors the open/close/error on
  bad use of the streams/writers
@joakime joakime requested a review from sbordet October 31, 2022 18:37
@joakime joakime self-assigned this Oct 31, 2022
catch (IOException ignore)
{
// not possible with InputStream.nullInputStream()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this try/catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because InputStream.close() throws IOException, and your InputStreamResponseListener.getInputStream() does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@joakime I don't understand. There is no need to close this stream here. And InputStreamResponseListener.getInputStream() returns a plain InputStream whose close() throws IOException.

Copy link
Contributor Author

@joakime joakime Nov 1, 2022

Choose a reason for hiding this comment

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

The Javadoc for InputStreamListener.getInputStream() says ..

  • The method may be invoked only once; subsequent invocations will return a closed {@link InputStream}.

This makes sure that the API contract is still satisfied, and also ensures that use of this "second use" InputStream returned correctly produces IOExceptions on any of the read methods indicating that the stream is closed already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbordet bump, are you ok with this now? (the code changed here too, a better approach proposed by @gregw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ jshell
|  Welcome to JShell -- Version 11.0.16
|  For an introduction type: /help intro

jshell> InputStream.nullInputStream();
$1 ==> java.io.InputStream$1@3d646c37

jshell> $1.read()
$2 ==> -1

jshell> $1.close()

jshell> $1.read()
|  Exception java.io.IOException: Stream closed
|        at InputStream$1.ensureOpen (InputStream.java:86)
|        at InputStream$1.read (InputStream.java:98)
|        at (#4:1)

jshell> /exit
|  Goodbye

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ jshell
|  Welcome to JShell -- Version 11.0.16
|  For an introduction type: /help intro

jshell> new BufferedInputStream(new ByteArrayInputStream(new byte[] { 0x01 }))
$1 ==> java.io.BufferedInputStream@26a1ab54

jshell> $1.read()
$2 ==> 1

jshell> $1.close()

jshell> $1.read()
|  Exception java.io.IOException: Stream closed
|        at BufferedInputStream.getBufIfOpen (BufferedInputStream.java:176)
|        at BufferedInputStream.fill (BufferedInputStream.java:220)
|        at BufferedInputStream.read (BufferedInputStream.java:271)
|        at (#4:1)

jshell> /exit
|  Goodbye

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and ...

$ jshell
|  Welcome to JShell -- Version 11.0.16
|  For an introduction type: /help intro

jshell> var fi = new FileInputStream(new File("/tmp/foo.txt"))
fi ==> java.io.FileInputStream@26a1ab54

jshell> fi.read()
$2 ==> 70

jshell> fi.close()

jshell> fi.read()
|  Exception java.io.IOException: Stream Closed
|        at FileInputStream.read0 (Native Method)
|        at FileInputStream.read (FileInputStream.java:231)
|        at (#4:1)

jshell> /exit
|  Goodbye

@joakime joakime requested a review from sbordet November 1, 2022 18:53
catch (IOException ignore)
{
// not possible with InputStream.nullInputStream()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@joakime I don't understand. There is no need to close this stream here. And InputStreamResponseListener.getInputStream() returns a plain InputStream whose close() throws IOException.

@joakime joakime requested review from gregw and sbordet November 2, 2022 21:35
@joakime joakime merged commit 08c47f5 into jetty-12.0.x Nov 9, 2022
@joakime joakime deleted the jetty-12-use-jvm-null-outputstream branch November 9, 2022 13:06
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.

3 participants