On 05/12/2014 08:42, r...@apache.org wrote: > Author: remm > Date: Fri Dec 5 08:42:50 2014 > New Revision: 1643194
Short version: +1 on some. Against anything that is added with the sole purpose of passing a broken TCK test. A few questions with the general aim of reducing the number of system properties. > > URL: http://svn.apache.org/viewvc?rev=1643194&view=rev > Log: > Add configuration options for extensions, the origin header, periodic check > and buffer size. This restores the previous behavior, but adds some > flexibility. > > Modified: > tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java > tomcat/trunk/java/org/apache/tomcat/websocket/LocalStrings.properties > tomcat/trunk/java/org/apache/tomcat/websocket/TransformationFactory.java > tomcat/trunk/java/org/apache/tomcat/websocket/Util.java > tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java > > Modified: tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java?rev=1643194&r1=1643193&r2=1643194&view=diff > ============================================================================== > --- tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java (original) > +++ tomcat/trunk/java/org/apache/tomcat/websocket/Constants.java Fri Dec 5 > 08:42:50 2014 > @@ -43,7 +43,8 @@ public class Constants { > static final byte INTERNAL_OPCODE_FLUSH = 0x18; > > // Buffers > - static final int DEFAULT_BUFFER_SIZE = 8 * 1024; > + static final int DEFAULT_BUFFER_SIZE = > + > Integer.getInteger("org.apache.tomcat.websocket.DEFAULT_BUFFER_SIZE", 8 * > 1024); +1. > // Client connection > public static final String HOST_HEADER_NAME = "Host"; > @@ -60,6 +61,24 @@ public class Constants { > public static final String WS_EXTENSIONS_HEADER_NAME = > "Sec-WebSocket-Extensions"; > > + // Configuration for Origin header in client > + static final boolean ALWAYS_ADD_ORIGIN_HEADER = > + > Boolean.getBoolean("org.apache.tomcat.websocket.ALWAYS_ADD_ORIGIN_HEADER"); Do we need this? I'd be happy with DEFAULT_ORIGIN_HEADER_VALUE == "" -> Don't add an origin by default. > + static final boolean SET_TARGET_AS_ORIGIN_HEADER = > + > Boolean.getBoolean("org.apache.tomcat.websocket.SET_TARGET_AS_ORIGIN_HEADER"); This concept I still think is fundamentally wrong. Target != origin. I'd really like to avoid having this in the code base if at all possible. Can you get the TCK to pass with just the following option? > + static final String DEFAULT_ORIGIN_HEADER_VALUE = > + > System.getProperty("org.apache.tomcat.websocket.DEFAULT_ORIGIN_HEADER_VALUE", > "null"); > + > + // Configuration for background processing checks intervals > + static final int DEFAULT_PROCESS_PERIOD = > + > Integer.getInteger("org.apache.tomcat.websocket.DEFAULT_PROCESS_PERIOD", 10); +1. > + // Configuration for extensions > + static final boolean DISABLE_BUILTIN_EXTENSIONS = > + > Boolean.getBoolean("org.apache.tomcat.websocket.DISABLE_BUILTIN_EXTENSIONS"); > + static final boolean ALLOW_UNSUPPORTED_EXTENSIONS = > + > Boolean.getBoolean("org.apache.tomcat.websocket.ALLOW_UNSUPPORTED_EXTENSIONS"); > + I'm against adding both of the above solely to work around broken TCK tests. Do they have any real world use case? > public static final boolean STRICT_SPEC_COMPLIANCE = > Boolean.getBoolean( > "org.apache.tomcat.websocket.STRICT_SPEC_COMPLIANCE"); > @@ -67,9 +86,13 @@ public class Constants { > public static final List<Extension> INSTALLED_EXTENSIONS; > > static { > - List<Extension> installed = new ArrayList<>(1); > - installed.add(new WsExtension("permessage-deflate")); > - INSTALLED_EXTENSIONS = Collections.unmodifiableList(installed); > + if (DISABLE_BUILTIN_EXTENSIONS) { > + INSTALLED_EXTENSIONS = Collections.unmodifiableList(new > ArrayList<Extension>()); > + } else { > + List<Extension> installed = new ArrayList<>(1); > + installed.add(new WsExtension("permessage-deflate")); > + INSTALLED_EXTENSIONS = Collections.unmodifiableList(installed); > + } > } > > private Constants() { > > Modified: > tomcat/trunk/java/org/apache/tomcat/websocket/LocalStrings.properties > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/LocalStrings.properties?rev=1643194&r1=1643193&r2=1643194&view=diff > ============================================================================== > --- tomcat/trunk/java/org/apache/tomcat/websocket/LocalStrings.properties > (original) > +++ tomcat/trunk/java/org/apache/tomcat/websocket/LocalStrings.properties Fri > Dec 5 08:42:50 2014 > @@ -35,6 +35,8 @@ perMessageDeflate.duplicateParameter=Dup > perMessageDeflate.invalidWindowSize=An invalid windows of [{1}] size was > specified for [{0}]. Valid values are whole numbers from 8 to 15 inclusive. > perMessageDeflate.unknownParameter=An unknown extension parameter [{0}] was > defined > > +transformerFactory.unsupportedExtension=The extension [{0}] is not supported > + > util.notToken=An illegal extension parameter was specified with name [{0}] > and value [{1}] > util.invalidMessageHandler=The message handler provided does not have an > onMessage(Object) method > util.invalidType=Unable to coerce value [{0}] to type [{1}]. That type is > not supported. > > Modified: > tomcat/trunk/java/org/apache/tomcat/websocket/TransformationFactory.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/TransformationFactory.java?rev=1643194&r1=1643193&r2=1643194&view=diff > ============================================================================== > --- tomcat/trunk/java/org/apache/tomcat/websocket/TransformationFactory.java > (original) > +++ tomcat/trunk/java/org/apache/tomcat/websocket/TransformationFactory.java > Fri Dec 5 08:42:50 2014 > @@ -20,8 +20,12 @@ import java.util.List; > > import javax.websocket.Extension; > > +import org.apache.tomcat.util.res.StringManager; > + > public class TransformationFactory { > > + private static final StringManager sm = > StringManager.getManager(Constants.PACKAGE_NAME); > + > private static final TransformationFactory factory = new > TransformationFactory(); > > private TransformationFactory() { > @@ -37,6 +41,11 @@ public class TransformationFactory { > if (PerMessageDeflate.NAME.equals(name)) { > return PerMessageDeflate.negotiate(preferences, isServer); > } > - return null; > + if (Constants.ALLOW_UNSUPPORTED_EXTENSIONS) { > + return null; > + } else { > + throw new IllegalArgumentException( > + sm.getString("transformerFactory.unsupportedExtension", > name)); > + } > } > } > > Modified: tomcat/trunk/java/org/apache/tomcat/websocket/Util.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/Util.java?rev=1643194&r1=1643193&r2=1643194&view=diff > ============================================================================== > --- tomcat/trunk/java/org/apache/tomcat/websocket/Util.java (original) > +++ tomcat/trunk/java/org/apache/tomcat/websocket/Util.java Fri Dec 5 > 08:42:50 2014 > @@ -308,8 +308,7 @@ public class Util { > return Boolean.valueOf(value); > } else if (type.equals(byte.class) || type.equals(Byte.class)) { > return Byte.valueOf(value); > - } else if (value.length() == 1 && > - (type.equals(char.class) || type.equals(Character.class))) { > + } else if (type.equals(char.class) || type.equals(Character.class)) { > return Character.valueOf(value.charAt(0)); On first reading I didn't like this but it is consistent with coercion in the EL spec so there is precedence. I guess that makes me +0. > } else if (type.equals(double.class) || type.equals(Double.class)) { > return Double.valueOf(value); > > Modified: > tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java?rev=1643194&r1=1643193&r2=1643194&view=diff > ============================================================================== > --- tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java > (original) > +++ tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java > Fri Dec 5 08:42:50 2014 > @@ -119,7 +119,7 @@ public class WsWebSocketContainer > private int maxTextMessageBufferSize = Constants.DEFAULT_BUFFER_SIZE; > private volatile long defaultMaxSessionIdleTimeout = 0; > private int backgroundProcessCount = 0; > - private int processPeriod = 10; > + private int processPeriod = Constants.DEFAULT_PROCESS_PERIOD; > > > @Override > @@ -242,6 +242,23 @@ public class WsWebSocketContainer > sa = new InetSocketAddress(host, port); > } > > + // Origin header > + if (Constants.ALWAYS_ADD_ORIGIN_HEADER && > + !reqHeaders.containsKey(Constants.ORIGIN_HEADER_NAME)) { > + List<String> originValues = new ArrayList<>(1); > + if (Constants.SET_TARGET_AS_ORIGIN_HEADER) { > + StringBuilder originValue = new StringBuilder(); > + originValue.append(path.getHost()); > + if (port != -1) { > + originValue.append(':').append(port); > + } > + originValues.add(originValue.toString()); > + } else { > + originValues.add(Constants.DEFAULT_ORIGIN_HEADER_VALUE); > + } > + reqHeaders.put(Constants.ORIGIN_HEADER_NAME, originValues); > + } > + > ByteBuffer request = createRequest(path, reqHeaders); > > AsynchronousSocketChannel socketChannel; > @@ -838,7 +855,6 @@ public class WsWebSocketContainer > public void backgroundProcess() { > // This method gets called once a second. > backgroundProcessCount ++; > - > if (backgroundProcessCount >= processPeriod) { > backgroundProcessCount = 0; > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
