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

review WebSocket MessageWriter and MessageReader #4538

Closed
lachlan-roberts opened this issue Feb 3, 2020 · 1 comment · Fixed by #4588
Closed

review WebSocket MessageWriter and MessageReader #4538

lachlan-roberts opened this issue Feb 3, 2020 · 1 comment · Fixed by #4588
Assignees
Milestone

Comments

@lachlan-roberts
Copy link
Contributor

Jetty version
10.0.x

Description
MessageWriter and MessageOutputStream look inefficient and are duplicated in both the jetty API and javax API implementations.

@lachlan-roberts lachlan-roberts added this to the 10.0.x milestone Feb 3, 2020
@lachlan-roberts lachlan-roberts self-assigned this Feb 3, 2020
lachlan-roberts added a commit that referenced this issue Feb 6, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts changed the title review WebSocket MessageWriter and MessageOutputStream review WebSocket MessageWriter and MessageReader Feb 12, 2020
@lachlan-roberts
Copy link
Contributor Author

It would be good to implement MessageWriter with an OutputStreamWriter delegating to MessageOutputStream. However OutputStreamWriter uses CodingErrorAction.REPLACE and so will replace coding errors with a replacement character. But we really want CodingErrorAction.REPORT to get the coding errors as exceptions.

Invalid UTF-8 should be tested for both MessageReader and MessageWriter.

Example test for MessageWriterTest.

@Test
public void testInvalidUtf8Sequence() throws IOException, InterruptedException
{
    final CharsetEncoder utf8Encoder = UTF_8.newEncoder()
        .onUnmappableCharacter(CodingErrorAction.REPORT)
        .onMalformedInput(CodingErrorAction.REPORT);

    final String invalidUtf8String = "\uD800";
    assertThrows(MalformedInputException.class, () -> utf8Encoder.encode(CharBuffer.wrap(invalidUtf8String.toCharArray())));

    FrameCapture capture = new FrameCapture();
    capture.setOutputBufferSize(1024);

    MessageWriter writer = new MessageWriter(capture, bufferPool);
    writer.write(invalidUtf8String.toCharArray());
    assertThrows(MalformedInputException.class, writer::flush);
}

lachlan-roberts added a commit that referenced this issue Feb 17, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 17, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 17, 2020
Message reader now validates UTF8

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 17, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 19, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 19, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 19, 2020
Message reader now validates UTF8

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 19, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 19, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Feb 19, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Mar 11, 2020
…Writer

Issue #4538 - rework of websocket message reader and writers
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 a pull request may close this issue.

1 participant