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

jetty-10.0.x WebSocket core cleanup #3783

Merged
merged 4 commits into from
Jun 21, 2019
Merged

jetty-10.0.x WebSocket core cleanup #3783

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7106dd
cleanups of WebSocket core
lachlan-roberts Jun 14, 2019
83362c5
WebSocket ConfigurationCustomizer now extends ConfigurationHolder
lachlan-roberts Jun 20, 2019
417fb0e
Merge remote-tracking branch 'jetty-10.0.x' into jetty-10.0.x-WebSock…
lachlan-roberts Jun 20, 2019
72f971f
adding isDebugEnabled checks
lachlan-roberts Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions ...websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/FrameHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,16 +519,16 @@ interface Customizer
void customize(Configuration configurable);
}

class ConfigurationCustomizer implements Customizer, Configuration
class ConfigurationHolder implements Configuration
{
private Duration idleTimeout;
private Duration writeTimeout;
private Boolean autoFragment;
private Long maxFrameSize;
private Integer outputBufferSize;
private Integer inputBufferSize;
private Long maxBinaryMessageSize;
private Long maxTextMessageSize;
protected Duration idleTimeout;
protected Duration writeTimeout;
protected Boolean autoFragment;
protected Long maxFrameSize;
protected Integer outputBufferSize;
protected Integer inputBufferSize;
protected Long maxBinaryMessageSize;
protected Long maxTextMessageSize;

@Override
public Duration getIdleTimeout()
Expand Down Expand Up @@ -625,7 +625,10 @@ public void setMaxTextMessageSize(long maxTextMessageSize)
{
this.maxTextMessageSize = maxTextMessageSize;
}
}

class ConfigurationCustomizer extends ConfigurationHolder implements Customizer
{
@Override
public void customize(Configuration configurable)
{
Expand Down
18 changes: 16 additions & 2 deletions ...ebsocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/ExtensionStack.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public class ExtensionStack implements IncomingFrames, OutgoingFrames, Dumpable
private List<Extension> extensions;
private IncomingFrames incoming;
private OutgoingFrames outgoing;
private String[] rsvClaims = new String[3];
gregw marked this conversation as resolved.
Show resolved Hide resolved

public ExtensionStack(WebSocketExtensionRegistry factory, Behavior behavior)
{
Expand Down Expand Up @@ -122,8 +123,6 @@ public void negotiate(DecoratedObjectFactory objectFactory, ByteBufferPool buffe

this.extensions = new ArrayList<>();

String rsvClaims[] = new String[3];

for (ExtensionConfig config : negotiatedConfigs)
{
Extension ext;
Copy link
Contributor

Choose a reason for hiding this comment

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

the debug logs below (a fair way) should be protected with if (LOG.isDebugEnabled ...

Expand Down Expand Up @@ -262,6 +261,21 @@ public void initialize(IncomingFrames incoming, OutgoingFrames outgoing, WebSock
extension.setWebSocketCoreSession(coreSession);
}

public boolean isRsv1Used()
{
return (rsvClaims[0] != null);
}

public boolean isRsv2Used()
{
return (rsvClaims[1] != null);
}

public boolean isRsv3Used()
{
return (rsvClaims[2] != null);
}

@Override
public String dump()
{
Expand Down
16 changes: 11 additions & 5 deletions ...socket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import org.eclipse.jetty.util.log.Logger;
import org.eclipse.jetty.websocket.core.CloseStatus;
import org.eclipse.jetty.websocket.core.Frame;
import org.eclipse.jetty.websocket.core.FrameHandler.Configuration;
import org.eclipse.jetty.websocket.core.FrameHandler.ConfigurationHolder;
import org.eclipse.jetty.websocket.core.MessageTooLargeException;
import org.eclipse.jetty.websocket.core.OpCode;
import org.eclipse.jetty.websocket.core.ProtocolException;
Expand All @@ -51,7 +53,7 @@ private enum State

private static final Logger LOG = Log.getLogger(Parser.class);
private final ByteBufferPool bufferPool;
private final boolean autoFragment;
private final Configuration configuration;

// State specific
private State state = State.START;
Expand All @@ -63,13 +65,13 @@ private enum State

public Parser(ByteBufferPool bufferPool)
{
this(bufferPool, true);
this(bufferPool, new ConfigurationHolder());
}

public Parser(ByteBufferPool bufferPool, boolean autoFragment)
public Parser(ByteBufferPool bufferPool, Configuration configuration)
{
this.bufferPool = bufferPool;
this.autoFragment = autoFragment;
this.configuration = configuration;
}

public void reset()
Expand Down Expand Up @@ -253,6 +255,10 @@ protected void checkFrameSize(byte opcode, int payloadLength) throws MessageTooL
{
if (OpCode.isControlFrame(opcode) && payloadLength > Frame.MAX_CONTROL_PAYLOAD)
throw new ProtocolException("Invalid control frame payload length, [" + payloadLength + "] cannot exceed [" + Frame.MAX_CONTROL_PAYLOAD + "]");

long maxFrameSize = configuration.getMaxFrameSize();
if (!configuration.isAutoFragment() && maxFrameSize > 0 && payloadLength > maxFrameSize)
throw new MessageTooLargeException("Cannot handle payload lengths larger than " + maxFrameSize);
}

protected ParsedFrame newFrame(byte firstByte, byte[] mask, ByteBuffer payload, boolean releaseable)
Expand Down Expand Up @@ -287,7 +293,7 @@ private ParsedFrame parsePayload(ByteBuffer buffer)
// not enough to complete this frame

// Can we auto-fragment
if (autoFragment && OpCode.isDataFrame(OpCode.getOpCode(firstByte)))
if (configuration.isAutoFragment() && OpCode.isDataFrame(OpCode.getOpCode(firstByte)))
{
payloadLength -= available;

Expand Down
33 changes: 1 addition & 32 deletions ...ket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@
import org.eclipse.jetty.util.thread.Scheduler;
import org.eclipse.jetty.websocket.core.Behavior;
import org.eclipse.jetty.websocket.core.Frame;
import org.eclipse.jetty.websocket.core.MessageTooLargeException;
import org.eclipse.jetty.websocket.core.ProtocolException;
import org.eclipse.jetty.websocket.core.WebSocketTimeoutException;

/**
Expand Down Expand Up @@ -83,23 +81,6 @@ public WebSocketConnection(EndPoint endp,
Scheduler scheduler,
ByteBufferPool bufferPool,
WebSocketCoreSession coreSession)
{
this(endp, executor, scheduler, bufferPool, coreSession, true);
}

/**
* Create a WSConnection.
* <p>
* It is assumed that the WebSocket Upgrade Handshake has already
* completed successfully before creating this connection.
* </p>
*/
public WebSocketConnection(EndPoint endp,
Executor executor,
Scheduler scheduler,
ByteBufferPool bufferPool,
WebSocketCoreSession coreSession,
boolean validating)
{
super(endp, executor);

Expand All @@ -113,18 +94,7 @@ public WebSocketConnection(EndPoint endp,
this.coreSession = coreSession;

this.generator = new Generator(bufferPool);
this.parser = new Parser(bufferPool, coreSession.isAutoFragment())
{
@Override
protected void checkFrameSize(byte opcode, int payloadLength) throws MessageTooLargeException, ProtocolException
{
super.checkFrameSize(opcode, payloadLength);
if (!coreSession.isAutoFragment() && coreSession.getMaxFrameSize() > 0 && payloadLength > coreSession.getMaxFrameSize())
throw new MessageTooLargeException("Cannot handle payload lengths larger than " + coreSession.getMaxFrameSize());
}

};

this.parser = new Parser(bufferPool, coreSession);
this.flusher = new Flusher(scheduler, coreSession.getOutputBufferSize(), generator, endp);
this.setInputBufferSize(coreSession.getInputBufferSize());

Expand Down Expand Up @@ -179,7 +149,6 @@ public void onClose(Throwable cause)
super.onClose(cause);
}


@Override
public boolean onIdleExpired()
{
Expand Down
82 changes: 23 additions & 59 deletions ...et-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketCoreSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import org.eclipse.jetty.websocket.core.Behavior;
import org.eclipse.jetty.websocket.core.CloseException;
import org.eclipse.jetty.websocket.core.CloseStatus;
import org.eclipse.jetty.websocket.core.Extension;
import org.eclipse.jetty.websocket.core.ExtensionConfig;
import org.eclipse.jetty.websocket.core.Frame;
import org.eclipse.jetty.websocket.core.FrameHandler;
Expand Down Expand Up @@ -81,9 +80,7 @@ public class WebSocketCoreSession implements IncomingFrames, FrameHandler.CoreSe
private Duration idleTimeout = WebSocketConstants.DEFAULT_IDLE_TIMEOUT;
private Duration writeTimeout = WebSocketConstants.DEFAULT_WRITE_TIMEOUT;

public WebSocketCoreSession(FrameHandler handler,
Behavior behavior,
Negotiated negotiated)
public WebSocketCoreSession(FrameHandler handler, Behavior behavior, Negotiated negotiated)
{
this.handler = handler;
this.behavior = behavior;
Expand All @@ -102,9 +99,9 @@ public boolean isDemanding()

public void assertValidIncoming(Frame frame)
{
assertValid(frame);
assertValidFrame(frame);

// Assert Behavior Required by RFC-6455 / Section 5.1
// Assert Incoming Frame Behavior Required by RFC-6455 / Section 5.1
switch (behavior)
{
case SERVER:
Expand All @@ -121,41 +118,30 @@ public void assertValidIncoming(Frame frame)

public void assertValidOutgoing(Frame frame) throws CloseException
{
// TODO check that it is not masked, since masking is done later
assertValidFrame(frame);
}

public void assertValidFrame(Frame frame)
{
if (!OpCode.isKnown(frame.getOpCode()))
throw new ProtocolException("Unknown opcode: " + frame.getOpCode());

assertValid(frame);

int payloadLength = (frame.getPayload() == null)?0:frame.getPayload().remaining();

if (frame.isControlFrame())
{
if (!frame.isFin())
throw new ProtocolException("Fragmented Control Frame [" + OpCode.name(frame.getOpCode()) + "]");

if (payloadLength > Frame.MAX_CONTROL_PAYLOAD)
throw new ProtocolException("Invalid control frame payload length, [" + payloadLength + "] cannot exceed [" + Frame.MAX_CONTROL_PAYLOAD + "]");
}
}

private void assertValid(Frame frame)
{

// Control Frame Validation
if (frame.isControlFrame())
{
if (frame.isRsv1())
throw new ProtocolException("Cannot have RSV1==true on Control frames");

if (frame.isRsv2())
throw new ProtocolException("Cannot have RSV2==true on Control frames");

if (frame.isRsv3())
throw new ProtocolException("Cannot have RSV3==true on Control frames");


/*
* RFC 6455 Section 5.5.1
* close frame payload is specially formatted which is checked in CloseStatus
Expand All @@ -168,42 +154,20 @@ private void assertValid(Frame frame)
}
else
{
// TODO should we validate UTF-8 for text frames
if (frame.getOpCode() == OpCode.TEXT)
{
}
}

/*
* RFC 6455 Section 5.2
*
* MUST be 0 unless an extension is negotiated that defines meanings for non-zero values. If a nonzero value is received and none of the negotiated
* extensions defines the meaning of such a nonzero value, the receiving endpoint MUST _Fail the WebSocket Connection_.
*/
//TODO save these values to not iterate through extensions every frame
List<? extends Extension> exts = getExtensionStack().getExtensions();

boolean isRsv1InUse = false;
boolean isRsv2InUse = false;
boolean isRsv3InUse = false;
for (Extension ext : exts)
{
if (ext.isRsv1User())
isRsv1InUse = true;
if (ext.isRsv2User())
isRsv2InUse = true;
if (ext.isRsv3User())
isRsv3InUse = true;
/*
* RFC 6455 Section 5.2
*
* MUST be 0 unless an extension is negotiated that defines meanings for non-zero values. If a nonzero value is received and none of the negotiated
* extensions defines the meaning of such a nonzero value, the receiving endpoint MUST _Fail the WebSocket Connection_.
*/
ExtensionStack extensionStack = negotiated.getExtensions();
if (frame.isRsv1() && !extensionStack.isRsv1Used())
throw new ProtocolException("RSV1 not allowed to be set");
if (frame.isRsv2() && !extensionStack.isRsv2Used())
throw new ProtocolException("RSV2 not allowed to be set");
if (frame.isRsv3() && !extensionStack.isRsv3Used())
throw new ProtocolException("RSV3 not allowed to be set");
}

if (frame.isRsv1() && !isRsv1InUse)
throw new ProtocolException("RSV1 not allowed to be set");

if (frame.isRsv2() && !isRsv2InUse)
throw new ProtocolException("RSV2 not allowed to be set");

if (frame.isRsv3() && !isRsv3InUse)
throw new ProtocolException("RSV3 not allowed to be set");
}

public ExtensionStack getExtensionStack()
Expand Down Expand Up @@ -491,12 +455,12 @@ public void demand(long n)

public WebSocketConnection getConnection()
{
return this.connection;
return connection;
}

public Executor getExecutor()
{
return this.connection.getExecutor();
return connection.getExecutor();
}

@Override
Expand Down Expand Up @@ -799,7 +763,7 @@ public String toString()
return String.format("WSCoreSession@%x{%s,%s,%s,af=%b,i/o=%d/%d,fs=%d}->%s",
hashCode(),
behavior,
sessionState,
sessionState,
negotiated,
autoFragment,
inputBufferSize,
Expand Down
5 changes: 2 additions & 3 deletions ...ket-core/src/test/java/org/eclipse/jetty/websocket/core/GeneratorParserRoundtripTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.eclipse.jetty.io.MappedByteBufferPool;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.websocket.core.internal.Generator;
import org.eclipse.jetty.websocket.core.internal.Parser;
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
Expand All @@ -41,7 +40,7 @@ public class GeneratorParserRoundtripTest
public void testParserAndGenerator() throws Exception
{
Generator gen = new Generator(bufferPool);
ParserCapture capture = new ParserCapture(new Parser(bufferPool));
ParserCapture capture = new ParserCapture();

String message = "0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF";

Expand Down Expand Up @@ -74,7 +73,7 @@ public void testParserAndGenerator() throws Exception
public void testParserAndGeneratorMasked() throws Exception
{
Generator gen = new Generator(bufferPool);
ParserCapture capture = new ParserCapture(new Parser(bufferPool), true, Behavior.SERVER);
ParserCapture capture = new ParserCapture(true, Behavior.SERVER);

String message = "0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF";

Expand Down
3 changes: 1 addition & 2 deletions ...ebsocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/GeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import org.eclipse.jetty.websocket.core.internal.ExtensionStack;
import org.eclipse.jetty.websocket.core.internal.Generator;
import org.eclipse.jetty.websocket.core.internal.Negotiated;
import org.eclipse.jetty.websocket.core.internal.Parser;
import org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -1230,7 +1229,7 @@ public void testGenerate_WithMasking() throws Exception
assertThat("Generated Buffer", completeBuffer.remaining(), is(expectedSize));

// Parse complete buffer.
ParserCapture capture = new ParserCapture(new Parser(new MappedByteBufferPool()), true, Behavior.SERVER);
ParserCapture capture = new ParserCapture(true, Behavior.SERVER);

capture.parse(completeBuffer);

Expand Down
Loading