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

ServletOutput.println violates Async Writer contract #228

Closed
gregw opened this issue Dec 18, 2018 · 4 comments
Closed

ServletOutput.println violates Async Writer contract #228

gregw opened this issue Dec 18, 2018 · 4 comments

Comments

@gregw
Copy link
Contributor

gregw commented Dec 18, 2018

The ServletOutput.println methods are implemented as follows:

    public void println(String s) throws IOException {
        print(s);
        println();
    }

This violates the API contract for asynchronous writers because isReady() needs to be called before every write/print. In this case the print(s) may fill a buffer, which is flushed and hits TCP/IP congestion. The output stream is now not ready, but the println() call is made regardless!

@gregw gregw changed the title ServletOutput.println violates Async Writer ServletOutput.println violates Async Writer contract Dec 18, 2018
@joakime
Copy link

joakime commented Dec 18, 2018

Couldn't the implementation be changed (or as a workaround, be overridden) to ...

    public void println(String s) throws IOException {
        print(s + System.lineSeparator());
    }

@gregw
Copy link
Contributor Author

gregw commented Dec 18, 2018

@joakime that would work, although it's not that efficient.... it would be better to add the line separator as the string is converted to a byte arrray.... oh wait there is another problem with print(String) as it does the string to byte conversion character by character, with write(byte) calls inbetween. This also violates the async write contract!

@stuartwdouglas
Copy link
Contributor

I think having less efficient but technically correct implementations is an ok compromise, as implementors can always override to whatever is most efficient for their implementations.

@gregw
Copy link
Contributor Author

gregw commented Dec 19, 2018

Good point! I'll prepare a PR with some minimal correct implementations

gregw added a commit to gregw/servlet-api that referenced this issue Dec 19, 2018
Make ServletOutputStream print methods compatible with async writing
gregw added a commit to gregw/servlet-api that referenced this issue Dec 19, 2018
Fixed formatting
@gregw gregw closed this as completed in 5495a5b Dec 24, 2018
gregw added a commit that referenced this issue Dec 24, 2018
sbordet added a commit to jetty/jetty.project that referenced this issue Dec 19, 2019
Removed methods that were overridden to
workaround jakartaee/servlet#228
in servlet-api 4.0.2, but that are now
fixed in servlet-api 4.0.3.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
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

No branches or pull requests

3 participants