[GitHub] [tomcat] michael-o commented on a change in pull request #382: Add support for unix domain sockets.

[GitBox]

michael-o commented on a change in pull request #382:
URL: https://github.com/apache/tomcat/pull/382#discussion_r532256127



##
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##
@@ -136,6 +138,19 @@ public static boolean setProperty(Object o, String name, 
String value,
 if (actualMethod != null) {
 
actualMethod.append(method.getName()).append("(InetAddress.getByName(\"").append(value).append("\"))");
 }
+// Try a setFoo ( Path )
+} else if ("java.nio.file.Path".equals(paramType
+.getName())) {
+try {
+params[0] = Paths.get(value);
+} catch (InvalidPathException ipe) {
+if (log.isDebugEnabled())
+log.debug("IntrospectionUtils: Unable to 
resolve path:" + value);

Review comment:
   Use the common `[..]` pattern

##
File path: java/org/apache/tomcat/util/net/AprEndpoint.java
##
@@ -292,52 +295,79 @@ public void bind() throws Exception {
 
 // Create the pool for the server socket
 serverSockPool = Pool.create(rootPool);
+
 // Create the APR address that will be bound
-String addressStr = null;
-if (getAddress() != null) {
-addressStr = getAddress().getHostAddress();
-}
-int family = Socket.APR_INET;
-if (Library.APR_HAVE_IPV6) {
-if (addressStr == null) {
-if (!OS.IS_BSD) {
+if (getPath() != null) {
+if (Library.APR_HAVE_UNIX) {
+hostname = getPath().toString();
+family = Socket.APR_UNIX;
+}
+else {
+throw new 
Exception(sm.getString("endpoint.init.unixnotavail"));
+}
+}
+else {
+
+if (getAddress() != null) {
+hostname = getAddress().getHostAddress();
+}
+family = Socket.APR_INET;
+if (Library.APR_HAVE_IPV6) {
+if (hostname == null) {
+if (!OS.IS_BSD) {
+family = Socket.APR_UNSPEC;
+}
+} else if (hostname.indexOf(':') >= 0) {
 family = Socket.APR_UNSPEC;
 }
-} else if (addressStr.indexOf(':') >= 0) {
-family = Socket.APR_UNSPEC;
 }
- }
+}
+
+long sockAddress = Address.info(hostname, family, getPortWithOffset(), 
0, rootPool);
 
-long inetAddress = Address.info(addressStr, family, 
getPortWithOffset(), 0, rootPool);
 // Create the APR server socket
-serverSock = Socket.create(Address.getInfo(inetAddress).family,
+if (family == Socket.APR_UNIX) {
+serverSock = Socket.create(family, Socket.SOCK_STREAM, 0, 
rootPool);
+}
+else {
+serverSock = Socket.create(Address.getInfo(sockAddress).family,
 Socket.SOCK_STREAM,
 Socket.APR_PROTO_TCP, rootPool);
-if (OS.IS_UNIX) {
-Socket.optSet(serverSock, Socket.APR_SO_REUSEADDR, 1);
-}
-if (Library.APR_HAVE_IPV6) {
-if (getIpv6v6only()) {
-Socket.optSet(serverSock, Socket.APR_IPV6_V6ONLY, 1);
-} else {
-Socket.optSet(serverSock, Socket.APR_IPV6_V6ONLY, 0);
+if (OS.IS_UNIX) {
+Socket.optSet(serverSock, Socket.APR_SO_REUSEADDR, 1);
+}
+if (Library.APR_HAVE_IPV6) {
+if (getIpv6v6only()) {
+Socket.optSet(serverSock, Socket.APR_IPV6_V6ONLY, 1);
+} else {
+Socket.optSet(serverSock, Socket.APR_IPV6_V6ONLY, 0);
+}
 }
+// Deal with the firewalls that tend to drop the inactive sockets
+Socket.optSet(serverSock, Socket.APR_SO_KEEPALIVE, 1);
 }
-// Deal with the firewalls that tend to drop the inactive sockets
-Socket.optSet(serverSock, Socket.APR_SO_KEEPALIVE, 1);
+
 // Bind the server socket
-int ret = Socket.bind(serverSock, inetAddress);
+int ret = Socket.bind(serverSock, sockAddress);
 if (ret != 0) {
 throw new Exception(sm.getString("endpoint.init.bind", "" + ret, 
Error.strerror(ret)));
 }
+
 // Start listening on the server socket
 ret = Socket.listen(serverSock, getAcceptCount());
 if (ret != 0) {
 throw new Exception(sm.getString("endpoint.init.listen", "" + ret, 
Error.strerror(ret)));
 }
-if (OS.IS_WIN32 || OS.IS_WIN64) {
-// On Windows set the reuseaddr flag after the bind/listen
-Socket.optSet(serverSock, Socket.APR_SO_REUSEADDR, 1);
+
+if 

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532255441



##
File path: native/src/network.c
##
@@ -79,6 +80,17 @@ static apr_status_t sp_socket_cleanup(void *data)
 (*s->net->cleanup)(s->opaque);
 if (s->sock) {
 apr_socket_t *as = s->sock;
+#ifdef APR_UNIX
+apr_sockaddr_t *sa = NULL;
+apr_socket_addr_get(, APR_LOCAL, as);
+if (sa && sa->family == APR_UNIX) {
+char *sock;
+apr_getnameinfo(, sa, 0);
+if (sock) {
+apr_file_remove(sock, s->pool);

Review comment:
   This is an interesting one, the file is deleted *before* the socket is 
closed?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532255441



##
File path: native/src/network.c
##
@@ -79,6 +80,17 @@ static apr_status_t sp_socket_cleanup(void *data)
 (*s->net->cleanup)(s->opaque);
 if (s->sock) {
 apr_socket_t *as = s->sock;
+#ifdef APR_UNIX
+apr_sockaddr_t *sa = NULL;
+apr_socket_addr_get(, APR_LOCAL, as);
+if (sa && sa->family == APR_UNIX) {
+char *sock;
+apr_getnameinfo(, sa, 0);
+if (sock) {
+apr_file_remove(sock, s->pool);

Review comment:
   This isn't an interesting one, the file is deleted *before* the socket 
is closed?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532254789



##
File path: native/include/tcn.h
##
@@ -281,11 +281,20 @@ typedef struct {
 #define APR_INET6 APR_INET
 #endif
 
+#ifdef APR_UNIX
 #define GET_S_FAMILY(T, F)   \
 if (F == 0) T = APR_UNSPEC;  \
 else if (F == 1) T = APR_INET;   \
 else if (F == 2) T = APR_INET6;  \
+else if (F == 3) T = APR_UNIX;   \

Review comment:
   Right, I completely missed this one that this is a macro. Apologies.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532254706



##
File path: java/org/apache/tomcat/jni/Address.java
##
@@ -40,8 +40,9 @@
 
 /**
  * Create apr_sockaddr_t from hostname, address family, and port.
- * @param hostname The hostname or numeric address string to 
resolve/parse, or
- *   NULL to build an address that corresponds to 0.0.0.0 or ::
+ * @param hostname The hostname or numeric address string to 
resolve/parse, the

Review comment:
   OK, consistency makes sense. I wonder why this has not been addressed in 
1.7 or 2.0.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] minfrin commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

minfrin commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532253991



##
File path: native/src/network.c
##
@@ -79,6 +80,15 @@ static apr_status_t sp_socket_cleanup(void *data)
 (*s->net->cleanup)(s->opaque);
 if (s->sock) {
 apr_socket_t *as = s->sock;
+apr_sockaddr_t *sa = NULL;
+apr_socket_addr_get(, APR_LOCAL, as);
+if (sa && sa->family == APR_UNIX) {

Review comment:
   This one is a mistake.
   





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] minfrin commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

minfrin commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532253902



##
File path: native/src/info.c
##
@@ -198,6 +198,9 @@ static void fill_ainfo(JNIEnv *e, jobject obj, 
apr_sockaddr_t *info)
 if (info->family == APR_UNSPEC) f = 0;
 else if (info->family == APR_INET) f = 1;
 else if (info->family == APR_INET6) f = 2;
+#ifdef APR_UNIX
+else if (info->family == APR_UNIX) f = 3;

Review comment:
   This is code rather than a macro.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] minfrin commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

minfrin commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532251725



##
File path: native/include/tcn.h
##
@@ -281,11 +281,20 @@ typedef struct {
 #define APR_INET6 APR_INET
 #endif
 
+#ifdef APR_UNIX
 #define GET_S_FAMILY(T, F)   \
 if (F == 0) T = APR_UNSPEC;  \
 else if (F == 1) T = APR_INET;   \
 else if (F == 2) T = APR_INET6;  \
+else if (F == 3) T = APR_UNIX;   \

Review comment:
   My compiler doesn't like this one - doesn't seem possible to put an 
ifdef inside a macro.
   





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] minfrin commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

minfrin commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532251226



##
File path: java/org/apache/tomcat/jni/Address.java
##
@@ -40,8 +40,9 @@
 
 /**
  * Create apr_sockaddr_t from hostname, address family, and port.
- * @param hostname The hostname or numeric address string to 
resolve/parse, or
- *   NULL to build an address that corresponds to 0.0.0.0 or ::
+ * @param hostname The hostname or numeric address string to 
resolve/parse, the

Review comment:
   This call matches the signature of the APR function, which stayed the 
same:
   
   
http://apr.apache.org/docs/apr/1.6/group__apr__network__io.html#gaa2f399ca2b60b35c0abf7630298c6c9f
   
   My feeling is to keep this aligned, even though "hostname" is technically 
incorrect.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] minfrin opened a new pull request #382: Add support for unix domain sockets.

[GitBox]

minfrin opened a new pull request #382:
URL: https://github.com/apache/tomcat/pull/382


   First implementation added to
   org.apache.coyote.http11.Http11AprProtocol.
   
   Depends on https://github.com/apache/tomcat-native/pull/8.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532249132



##
File path: java/org/apache/tomcat/jni/Address.java
##
@@ -40,8 +40,9 @@
 
 /**
  * Create apr_sockaddr_t from hostname, address family, and port.
- * @param hostname The hostname or numeric address string to 
resolve/parse, or
- *   NULL to build an address that corresponds to 0.0.0.0 or ::
+ * @param hostname The hostname or numeric address string to 
resolve/parse, the

Review comment:
   I think the generic name is address which represents `hostname:port`,  
`IPv4:port' , `[IPv6]:port`, '/path`. Should we deprecate this one and replace 
with a new one?

##
File path: native/src/info.c
##
@@ -198,6 +198,9 @@ static void fill_ainfo(JNIEnv *e, jobject obj, 
apr_sockaddr_t *info)
 if (info->family == APR_UNSPEC) f = 0;
 else if (info->family == APR_INET) f = 1;
 else if (info->family == APR_INET6) f = 2;
+#ifdef APR_UNIX
+else if (info->family == APR_UNIX) f = 3;

Review comment:
   Just like here...

##
File path: native/src/network.c
##
@@ -79,6 +80,15 @@ static apr_status_t sp_socket_cleanup(void *data)
 (*s->net->cleanup)(s->opaque);
 if (s->sock) {
 apr_socket_t *as = s->sock;
+apr_sockaddr_t *sa = NULL;
+apr_socket_addr_get(, APR_LOCAL, as);
+if (sa && sa->family == APR_UNIX) {

Review comment:
   If you guard all the rest with `#ifdef APR_UNIX` why not this one?

##
File path: native/include/tcn.h
##
@@ -281,11 +281,20 @@ typedef struct {
 #define APR_INET6 APR_INET
 #endif
 
+#ifdef APR_UNIX
 #define GET_S_FAMILY(T, F)   \
 if (F == 0) T = APR_UNSPEC;  \
 else if (F == 1) T = APR_INET;   \
 else if (F == 2) T = APR_INET6;  \
+else if (F == 3) T = APR_UNIX;   \

Review comment:
   Why not make the ifdef around this line?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] minfrin commented on pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

minfrin commented on pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#issuecomment-735430773


   Bugzilla here: https://bz.apache.org/bugzilla/show_bug.cgi?id=64942



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] minfrin opened a new pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

[GitBox]

minfrin opened a new pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8


   This patch adds support for Unix Domain Sockets, that was added to APR v1.6.
   
   This consists of the addition of the APR_UNIX family, as well as adding a 
cleanup to remove the socket file on shutdown.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] Sumithkr opened a new pull request #381: Create Documentation installing tomcat on ibm cloud

[GitBox]

Sumithkr opened a new pull request #381:
URL: https://github.com/apache/tomcat/pull/381


   Add this document on documentation page 
   [Tomcat.docx](https://github.com/apache/tomcat/files/5612520/Tomcat.docx)
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf opened a new pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols

[GitBox]

markt-asf opened a new pull request #380:
URL: https://github.com/apache/tomcat/pull/380


   https://bz.apache.org/bugzilla/show_bug.cgi?id=64110
   
   I'm providing a PR for this one as the additional attributes to add some 
additional overhead. The overhead is limited to keeping two lists of strings: 
one of requested protocol names and one of requested cipher names. In both 
cases the names are already in memory so the footprint will be limited to the 
lists and array of object references. The exceptions are the relatively new 
GREASE values (see RFC 8701). I don't think they'll cause too much overhead 
(and there are ways to workaround that if they do - namely do exactly what RFC 
8701 doesn't want us to do and enumerate them).



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] pmouawad commented on pull request #376: Implement Bug 64877

[GitBox]

pmouawad commented on pull request #376:
URL: https://github.com/apache/tomcat/pull/376#issuecomment-734270073


   Thanks Remy !



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #379: Fix BZ 64080 - graceful close

[GitBox]

markt-asf closed pull request #379:
URL: https://github.com/apache/tomcat/pull/379


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] rmaucher commented on pull request #379: Fix BZ 64080 - graceful close

[GitBox]

rmaucher commented on pull request #379:
URL: https://github.com/apache/tomcat/pull/379#issuecomment-733693070


   Looks ok, at least I can't see right now why it would be bad.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on a change in pull request #379: Fix BZ 64080 - graceful close

[GitBox]

markt-asf commented on a change in pull request #379:
URL: https://github.com/apache/tomcat/pull/379#discussion_r530331565



##
File path: java/org/apache/tomcat/util/net/AbstractEndpoint.java
##
@@ -1312,6 +1340,30 @@ public final void closeServerSocketGraceful() {
 }
 
 
+/**
+ * Wait for the client connections to the server to close gracefully. The
+ * method will return when all of the client connections have closed or the
+ * method has been waiting for {@code waitTimeMillis}.
+ *
+ * @param waitMillisThe maximum time to wait in milliseconds for the
+ *  client connections to close.
+ *
+ * @return The wait time, if any remaining when the method returned
+ */
+public final long closeConnectionsAwait(long waitMillis) {

Review comment:
   Good suggestion. Fixed.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on a change in pull request #379: Fix BZ 64080 - graceful close

[GitBox]

markt-asf commented on a change in pull request #379:
URL: https://github.com/apache/tomcat/pull/379#discussion_r530331336



##
File path: java/org/apache/tomcat/util/net/AbstractEndpoint.java
##
@@ -1302,6 +1320,16 @@ protected long countDownConnection() {
  */
 public final void closeServerSocketGraceful() {
 if (bindState == BindState.BOUND_ON_START) {
+// Stop accepting new connections
+acceptor.stop(-1);
+// Release locks that may be preventing the acceptor from stopping
+releaseConnectionLatch();
+unlockAccept();
+// Signal to any multiplexed protocols (HTTP/2) that they may wish
+// to stop accepting new streams
+getHandler().pause();
+// Update the bindSatte. This has the side-effect of disabling

Review comment:
   Fixed. Tx for the review.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on a change in pull request #379: Fix BZ 64080 - graceful close

[GitBox]

martin-g commented on a change in pull request #379:
URL: https://github.com/apache/tomcat/pull/379#discussion_r530317113



##
File path: java/org/apache/tomcat/util/net/AbstractEndpoint.java
##
@@ -1302,6 +1320,16 @@ protected long countDownConnection() {
  */
 public final void closeServerSocketGraceful() {
 if (bindState == BindState.BOUND_ON_START) {
+// Stop accepting new connections
+acceptor.stop(-1);
+// Release locks that may be preventing the acceptor from stopping
+releaseConnectionLatch();
+unlockAccept();
+// Signal to any multiplexed protocols (HTTP/2) that they may wish
+// to stop accepting new streams
+getHandler().pause();
+// Update the bindSatte. This has the side-effect of disabling

Review comment:
   s/bindSatte/bindState/

##
File path: java/org/apache/tomcat/util/net/AbstractEndpoint.java
##
@@ -1312,6 +1340,30 @@ public final void closeServerSocketGraceful() {
 }
 
 
+/**
+ * Wait for the client connections to the server to close gracefully. The
+ * method will return when all of the client connections have closed or the
+ * method has been waiting for {@code waitTimeMillis}.
+ *
+ * @param waitMillisThe maximum time to wait in milliseconds for the
+ *  client connections to close.
+ *
+ * @return The wait time, if any remaining when the method returned
+ */
+public final long closeConnectionsAwait(long waitMillis) {

Review comment:
   Maybe the method name should be `awaitConnectionsClose` ?
   Now it sounds like it closes and waits, but actually it just waits.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf opened a new pull request #379: Fix BZ 64080 - graceful close

[GitBox]

markt-asf opened a new pull request #379:
URL: https://github.com/apache/tomcat/pull/379


   Fixes https://bz.apache.org/bugzilla/show_bug.cgi?id=64080
   
   Doing this via a PR to give folks a chance to review the changes first



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun closed pull request #373: lazy set filed `DELETE_FILES_SERVICE`

[GitBox]

dao-jun closed pull request #373:
URL: https://github.com/apache/tomcat/pull/373


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun closed pull request #378: use a single `Thread` to do `async-delete-file` works instead of `ThreadPoolExecutor`

[GitBox]

dao-jun closed pull request #378:
URL: https://github.com/apache/tomcat/pull/378


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #377: Set "Secure" session cookie attribute if called under SSL.

[GitBox]

markt-asf closed pull request #377:
URL: https://github.com/apache/tomcat/pull/377


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #377: Set "Secure" session cookie attribute if called under SSL.

[GitBox]

markt-asf commented on pull request #377:
URL: https://github.com/apache/tomcat/pull/377#issuecomment-730424887


   Thanks for the PR. I have implemented a variation of this in 10.0.x and will 
back-port to 9.0.x and 8.5.x.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #323: fix module names to reflect Jakarta EE origin

[GitBox]

markt-asf commented on pull request #323:
URL: https://github.com/apache/tomcat/pull/323#issuecomment-730329196


   This has been addressed in 10.0.x and 9.0.x. Note there nay be another 
change for 10.1.x depending on what naming convention Jakarta EE adopts.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #323: fix module names to reflect Jakarta EE origin

[GitBox]

markt-asf closed pull request #323:
URL: https://github.com/apache/tomcat/pull/323


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on a change in pull request #378: use a single `Thread` to do `async-delete-file` works instead of `ThreadPoolExecutor`

[GitBox]

martin-g commented on a change in pull request #378:
URL: https://github.com/apache/tomcat/pull/378#discussion_r526794048



##
File path: java/org/apache/juli/FileHandler.java
##
@@ -18,37 +18,20 @@
 
 package org.apache.juli;
 
-import java.io.BufferedOutputStream;
-import java.io.File;
-import java.io.FileOutputStream;
-import java.io.IOException;
-import java.io.OutputStream;
-import java.io.OutputStreamWriter;
-import java.io.PrintWriter;
-import java.io.UnsupportedEncodingException;
+import java.io.*;

Review comment:
   Please don't use wildcard imports

##
File path: java/org/apache/juli/FileHandler.java
##
@@ -58,98 +41,48 @@
  * The following configuration properties are available:
  *
  * 
- *   directory - The directory where to create the log file.
- *If the path is not absolute, it is relative to the current working
- *directory of the application. The Apache Tomcat configuration files 
usually
- *specify an absolute path for this property,
- *${catalina.base}/logs
- *Default value: logs
- *   rotatable - If true, the log file will be
- *rotated on the first write past midnight and the filename will be
- *{prefix}{date}{suffix}, where date is -MM-dd. If 
false,
- *the file will not be rotated and the filename will be 
{prefix}{suffix}.
- *Default value: true
- *   prefix - The leading part of the log file name.
- *Default value: juli.
- *   suffix - The trailing part of the log file name. Default 
value: .log
- *   bufferSize - Configures buffering. The value of 
0
- *uses system default buffering (typically an 8K buffer will be used). A
- *value of 0 forces a writer flush upon each log write. A
- *value 0 uses a BufferedOutputStream with the defined
- *value but note that the system default buffering will also be
- *applied. Default value: -1
- *   encoding - Character set used by the log file. Default 
value:
- *empty string, which means to use the system default character set.
- *   level - The level threshold for this Handler. See the
- *java.util.logging.Level class for the possible levels.
- *Default value: ALL
- *   filter - The java.util.logging.Filter
- *implementation class name for this Handler. Default value: unset
- *   formatter - The java.util.logging.Formatter
- *implementation class name for this Handler. Default value:
- *java.util.logging.SimpleFormatter
- *   maxDays - The maximum number of days to keep the log
- *files. If the specified value is =0 then the log files
- *will be kept on the file system forever, otherwise they will be kept the
- *specified maximum days. Default value: -1.
+ * directory - The directory where to create the log file.
+ * If the path is not absolute, it is relative to the current working
+ * directory of the application. The Apache Tomcat configuration files usually
+ * specify an absolute path for this property,
+ * ${catalina.base}/logs
+ * Default value: logs
+ * rotatable - If true, the log file will be
+ * rotated on the first write past midnight and the filename will be
+ * {prefix}{date}{suffix}, where date is -MM-dd. If 
false,
+ * the file will not be rotated and the filename will be 
{prefix}{suffix}.
+ * Default value: true
+ * prefix - The leading part of the log file name.
+ * Default value: juli.
+ * suffix - The trailing part of the log file name. Default 
value: .log
+ * bufferSize - Configures buffering. The value of 
0
+ * uses system default buffering (typically an 8K buffer will be used). A
+ * value of 0 forces a writer flush upon each log write. A
+ * value 0 uses a BufferedOutputStream with the defined
+ * value but note that the system default buffering will also be
+ * applied. Default value: -1
+ * encoding - Character set used by the log file. Default 
value:
+ * empty string, which means to use the system default character set.
+ * level - The level threshold for this Handler. See the
+ * java.util.logging.Level class for the possible levels.
+ * Default value: ALL
+ * filter - The java.util.logging.Filter
+ * implementation class name for this Handler. Default value: unset
+ * formatter - The java.util.logging.Formatter
+ * implementation class name for this Handler. Default value:
+ * java.util.logging.SimpleFormatter
+ * maxDays - The maximum number of days to keep the log
+ * files. If the specified value is =0 then the log files
+ * will be kept on the file system forever, otherwise they will be kept the
+ * specified maximum days. Default value: -1.

Review comment:
   Whitespace changes which are not really needed.

##
File path: java/org/apache/juli/FileHandler.java
##
@@ -585,4 +517,34 @@ private String obtainDateFromPath(Path path) {
 return null;
 }
 }
+
+
+static class AsyncTaskExecutor {
+
+private Thread thread;
+private BlockingQueue tasks;
+
+AsyncTaskExecutor() {
+this.tasks = 

[GitHub] [tomcat] dao-jun opened a new pull request #378: use a single `Thread` to do `async-delete-file` works instead of `ThreadPoolExecutor`

[GitBox]

dao-jun opened a new pull request #378:
URL: https://github.com/apache/tomcat/pull/378


   see: https://github.com/apache/tomcat/pull/373



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on a change in pull request #377: Set "Secure" session cookie attribute if called under SSL.

[GitBox]

martin-g commented on a change in pull request #377:
URL: https://github.com/apache/tomcat/pull/377#discussion_r526015818



##
File path: test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java
##
@@ -238,6 +238,8 @@ private void runValve(String jkActivation,
 expectedCookie.setPath(cookieConfig.getPath());
 expectedCookie.setMaxAge(0);
 
+EasyMock.expect(request.isSecure()).andReturn(true);

Review comment:
   Actually, I think we need a second test method to have coverage for the 
`cookieConfig.isSecure()` case. E.g. `runValve()` could have an additional 
boolean/enum parameter that decides whether to prepare `request.isSecure()` or 
`cookieConfig.isSecure()`.
   
   In addition I don't see any new code that verifies that the `expectedCookie` 
has `isSecure() == true`





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] akurth commented on a change in pull request #377: Set "Secure" session cookie attribute if called under SSL.

[GitBox]

akurth commented on a change in pull request #377:
URL: https://github.com/apache/tomcat/pull/377#discussion_r524478033



##
File path: test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java
##
@@ -238,6 +238,8 @@ private void runValve(String jkActivation,
 expectedCookie.setPath(cookieConfig.getPath());
 expectedCookie.setMaxAge(0);
 
+EasyMock.expect(request.isSecure()).andReturn(true);

Review comment:
   Feel free to add it. Makes no difference though, since || is 
short-circuit.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #377: Set "Secure" session cookie attribute if called under SSL.

[GitBox]

ChristopherSchultz commented on a change in pull request #377:
URL: https://github.com/apache/tomcat/pull/377#discussion_r524294255



##
File path: test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java
##
@@ -238,6 +238,8 @@ private void runValve(String jkActivation,
 expectedCookie.setPath(cookieConfig.getPath());
 expectedCookie.setMaxAge(0);
 
+EasyMock.expect(request.isSecure()).andReturn(true);

Review comment:
   Should this be `expect(request.isSecure() || cookieConfig.isSecure())`?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] jbampton commented on pull request #361: Add a GitHub action to lint the Markdown and YAML files.

[GitBox]

jbampton commented on pull request #361:
URL: https://github.com/apache/tomcat/pull/361#issuecomment-727913731


   Hey @markt-asf I have rebased and moved the two config files back to the 
repo root.
   
   The 2 files are not specific to GitHub so say we moved to GitLab we wouldn't 
want our whole repo config files in a GitHub folder.
   
   I have also now added the licenses to all new files.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] akurth opened a new pull request #377: Set "Secure" session cookie attribute if called under SSL.

[GitBox]

akurth opened a new pull request #377:
URL: https://github.com/apache/tomcat/pull/377


   Missing "Secure" flag may lead to redirect loops under certain
   conditions, e.g. when sameSiteCookies=none has been set at the
   CookieProcessor and browser is Chrome.
   
   Fixes .



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] rmaucher closed pull request #376: Implement Bug 64877

[GitBox]

rmaucher closed pull request #376:
URL: https://github.com/apache/tomcat/pull/376


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] rmaucher commented on pull request #376: Implement Bug 64877

[GitBox]

rmaucher commented on pull request #376:
URL: https://github.com/apache/tomcat/pull/376#issuecomment-724639920


   I meged the PR, sorf of, in Tomcat 10. Thanks ! Also added in 9.0 and 8.5.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] rmaucher commented on pull request #376: Implement Bug 64877

[GitBox]

rmaucher commented on pull request #376:
URL: https://github.com/apache/tomcat/pull/376#issuecomment-722999067


   At least for Tomcat 10, modifying the existing realm is the way to go. For 
example, the JDBCRealm was also removed over DataSourceRealm for similar 
reasons.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] pmouawad opened a new pull request #376: Implement Bug 64877

[GitBox]

pmouawad opened a new pull request #376:
URL: https://github.com/apache/tomcat/pull/376


   Hello,
   First thanks for your great product I've been using for years.
   
   Recently, I worked on a project that uses JDBCStore to store sessions.
   I had to run performance tests on it.
   Those tests revealed a high contention point in this component due to the 
coarse granularity of synchronization. 
   The impact was that 1 thread could block hundreds of others leading to high 
response time at higher scale.
   
   So I implemented (based on existing one) a new one with following 
differences:
   
   - I kept only the dataSourceName based configuration
   - I removed stateful instance variables in order to be able to lift 
synchronization
   
   This new implementation improved drastically performances.
   
   I am aware that there is duplication with JDBCStore that could be improved 
but it's a first shot and I lack of time unfortunately.
   
   I hope you can take into account this contribution.
   
   The associated bugzilla enhancement is:
   
   - https://bz.apache.org/bugzilla/show_bug.cgi?id=64877
   
   Regards
   Philippe M.
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #373: lazy set filed `DELETE_FILES_SERVICE`

[GitBox]

markt-asf commented on pull request #373:
URL: https://github.com/apache/tomcat/pull/373#issuecomment-720330295







This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun commented on pull request #373: lazy set filed `DELETE_FILES_SERVICE`

[GitBox]

dao-jun commented on pull request #373:
URL: https://github.com/apache/tomcat/pull/373#issuecomment-720880325


   > How does this fix the issue? It only delays the creation of the executor 
from the point the class is loaded to the point where the first instance is 
created. Instance creation is also triggered by the parsing of 
logging.properties and occurs immediately the class is loaded. The class 
loading is triggered by the instance creation so I don't see how this proposed 
PR can help.
   
   Yes, I just noticed it create an instance in 
`ClassLoaderLogManager#readConfiguration` by invoking 
`classLoader.loadClass(handlerClassName).getConstructor().newInstance()`, I'm 
sorry for that. How about using a single `Thread` instead of 
`ThreadPoolExecutor` ?



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] rmaucher commented on pull request #371: NIO2: Call `accept` immediately if connection count is not close to max con…

[GitBox]

rmaucher commented on pull request #371:
URL: https://github.com/apache/tomcat/pull/371#issuecomment-718885690


   Hum, ok, since incrementing is not done in parallel, then it's something 
that can be tried.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] anilgursel commented on pull request #371: NIO2: Call `accept` immediately if connection count is not close to max con…

[GitBox]

anilgursel commented on pull request #371:
URL: https://github.com/apache/tomcat/pull/371#issuecomment-718799285


   I updated the PR to have it as `getMaxConnections() / 2`.  I agree it is 
just a heuristic and not clean.  We can potentially make it configurable?
   
   If my understanding is correct, one reason to schedule the `accept` is to 
prevent blocking the thread.  The reason behind blocking (via `LimitLatch`) is 
to have a hard limit on the `maxConnections`.  Is there any possibility to 
relax this a bit?  Something like:
   
   ```java
   else if (getConnectionCount() < getMaxConnections()) {
   increment();
   serverSock.accept(null, this);
   }
   
   ```
   
   With this, there is possibility of number of connections to go over 
`maxConnections` little bit (which I think the expectation can be managed in 
documentation).  Actually, I believe there will only be one thread doing 
`accept` logic at any given time.  Because, a new `accept` is scheduled only 
when the previous one is `completed`.   Is this true?  If so,  it would not go 
beyond maxConnections.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] rmaucher commented on pull request #371: NIO2: Call `accept` immediately if connection count is not close to max con…

[GitBox]

rmaucher commented on pull request #371:
URL: https://github.com/apache/tomcat/pull/371#issuecomment-718632321


   maxConnections overall is a bad design for NIO2, that is a bit obvious. It 
started with connections not being counted accurately then after some iterative 
refactorings of close it remains a bad idea. For the longest time the default 
was fixed to -1 (still there in 8.5).
   
   I guess all you're doing is add a heuristic that works for you [but actually 
no guarantee]. Also about accept, I refactored it NIO2 style to save one thread 
and it seems more efficient overall, before it was a much more classical NIO1 
style accept using the same dedicated thread (so one extra thread, and likely 
less efficient). The downside of the strategy is that the accept is just like 
the other processing threads when maxConnections is used. If there's a heavy 
thread starvation, then accept will get delayed. I don't think it's the end of 
the world, the server will actually be slow for everyone, but I understand. 
OTOH, if your app is as you say using non blocking IO and is saving on threads, 
then there's no downside to the strategy [even with full use of the thread 
pool, the threads become available very frequently and accept stays responsive].
   
   So I'm ok with adding some heuristic, but maxThreads can be too low. Maybe 
maxConnections/2 would be good enough, with another heuristic on a minimal 
number ... Not very clean ...



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] CheerfulSrr closed pull request #374: 配置idea运行方式

[GitBox]

CheerfulSrr closed pull request #374:
URL: https://github.com/apache/tomcat/pull/374


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] CheerfulSrr opened a new pull request #374: 配置idea运行方式

[GitBox]

CheerfulSrr opened a new pull request #374:
URL: https://github.com/apache/tomcat/pull/374


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun commented on pull request #373: lazy set filed `DELETE_FILES_SERVICE`

[GitBox]

dao-jun commented on pull request #373:
URL: https://github.com/apache/tomcat/pull/373#issuecomment-714864911


   > Please provide more information what kind of issue you have now!
   > The description is a bit vague at the moment.
   
 We use `java agent` to enhance class `ThreadPoolExecutor` to provide 
`cross thread trace monitor ability`, but we found that class 
`ThreadPoolExecutor`  loaded before agent start, it caused by class 
`AsyncFileHandler` which defined in `logging.properties`.  So that `java agent` 
could not enhance class `ThreadPoolExecutor`.  Event we replaced 
`org.apache.juli.AsyncFileHandler` with `java.util.logging.FileHandler`  to 
avoid the issue, but I want to fix it.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on pull request #373: lazy set filed `DELETE_FILES_SERVICE`

[GitBox]

martin-g commented on pull request #373:
URL: https://github.com/apache/tomcat/pull/373#issuecomment-714587911


   Please provide more information what kind of issue you have now!
   The description is a bit vague at the moment.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun opened a new pull request #373: lazy set filed `DELETE_FILES_SERVICE`

[GitBox]

dao-jun opened a new pull request #373:
URL: https://github.com/apache/tomcat/pull/373


   Lazy set field `DELETE_FILES_SERVICE` to avoid class `ThreadPoolExecutor` 
loaded earlier than tomcat start.
   
   Actually, some `APM` systems enhance class `ThreadPoolExecutor` with `java 
agent`,  the field `DELETE_FILES_SERVICE` may cause enhance failed.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun commented on pull request #372: Update FileHandler.java

[GitBox]

dao-jun commented on pull request #372:
URL: https://github.com/apache/tomcat/pull/372#issuecomment-714342340


   > The DELETE_FILES_SERVICE executor is intended to be a singleton. This PR 
breaks that and is, therefore, unlikely to be applied.
   
   Yep, I forgot that. The pr will be closed and I would use inner class to 
keep the field singleton.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun closed pull request #372: Update FileHandler.java

[GitBox]

dao-jun closed pull request #372:
URL: https://github.com/apache/tomcat/pull/372


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #372: Update FileHandler.java

[GitBox]

markt-asf commented on pull request #372:
URL: https://github.com/apache/tomcat/pull/372#issuecomment-714333180


   The DELETE_FILES_SERVICE executor is intended to be a singleton. This PR 
breaks that and is, therefore, unlikely to be applied.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] dao-jun opened a new pull request #372: Update FileHandler.java

[GitBox]

dao-jun opened a new pull request #372:
URL: https://github.com/apache/tomcat/pull/372


   remove static keyword from  `DELETE_FILES_SERVICE` to avoid class 
`ThreadPoolExecutor` loaded earlier than agent start.
   It can cause agent enhance  class `ThreadPoolExecutor` failed.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] anilgursel opened a new pull request #371: Call `accept` immediately if connection count is not close to max con…

[GitBox]

anilgursel opened a new pull request #371:
URL: https://github.com/apache/tomcat/pull/371


   …nections
   
   Under regular load, we observe NIO2 has better CPU utilization and slightly 
better latency compared to NIO1.  However, under heavy load or during spikes, 
we observed that the 99th percentile latency of NIO2 is significantly worse 
compared to NIO1.   
   
   - We see that the latency is worse compared to NIO1 only for the requests 
that end up establishing a new connection.
   - When `maxKeepAliveRequests=-1`, we do not observe the latency degradation 
with NIO2 at all.  But, we need and use Tomcat’s `maxKeepAliveRequests` 
feature, so setting it to `-1` is not an option.  
   - The problem occurs only during spikes/high load, e.g., maxThreads=40, 
while the concurrent users sending requests is 100.
   - The issue happens only for blocking scenarios, where Tomcat threads are 
blocked during request processing.  When non-blocking programming is used 
(where Tomcat threads are freed quickly), we do not see any degradation 
compared to NIO1.  However, we have many applications that would not be able to 
adopt non-blocking anytime soon.
   
   
   While looking at Tomcat code, we saw that the `accept` is scheduled to be 
run on the thread pool.  Our hypothesis was that this scheduling was causing 
the `accept` to be delayed significantly under heavy load.  So, for a request 
with new connection establishment, it needs to go through the `TaskQueue` 
twice.  So, just for testing purpose we set `maxConnections=-1` to see the 
impact of immediate `accept` call (and in our test the number of concurrent 
connections does not get close to `maxConnections`), we observed that the 99th 
percentile latency problem with NIO2 disappeared.
   
   Accordingly, proposing a change to call `accept` immediately if the number 
of connections are not close to `maxConnections`.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] blueSky1825821 commented on pull request #368: fix: CachedResource add path length

[GitBox]

blueSky1825821 commented on pull request #368:
URL: https://github.com/apache/tomcat/pull/368#issuecomment-707629467


   > I've done some calculations and the absolute upper limit (the actual upper 
limit will be lower) is that a cache with a 10MB limit on size will actually 
use a little under 175MB. That isn't great but it isn't terrible either. I'll 
tweak the resource size calculation so it includes an approximate of the memory 
required to store the path.
   > The proposed patch has a couple of issues:
   > 
   > * It converts the string to bytes to determine the size.  This is both 
expensive and wrong. String length *2 +38 is a much closer approximation and I 
intend to ignore the 38 as that is already accounted for in the 500 byte 
overhead for each entry.
   > * It includes the path size when determining if the resource exceeds the 
max object size (and hence won't have the content cached) but does not include 
the path size when performing the same check to determine when deciding whether 
to cache the content or not.
   > 
   > I should have a fix for this committed shortly.
   
   Let me add that, after jdk8, e.g jdk9 String has been changed
   private final char value[]; -->  private final byte[] value; 
   but length *2 is also fine
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #370: Polish LocalStrings_zh_CN.properties

[GitBox]

markt-asf closed pull request #370:
URL: https://github.com/apache/tomcat/pull/370


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #370: Polish LocalStrings_zh_CN.properties

[GitBox]

markt-asf commented on pull request #370:
URL: https://github.com/apache/tomcat/pull/370#issuecomment-707210751


   Thanks for the PR. The Tomcat project manages i18n files via poeditor.com 
I've made these changes there (as well as several others I found) and I'll 
import them shortly.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] ijliym opened a new pull request #370: Polish LocalStrings_zh_CN.properties

[GitBox]

ijliym opened a new pull request #370:
URL: https://github.com/apache/tomcat/pull/370


   Remove unexpected '('.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #369: UDecoder does not properly convert when EncodedSolidusHandling is PASS_THROUGH and non-solidus encoded characters are present before the solidus

[GitBox]

markt-asf closed pull request #369:
URL: https://github.com/apache/tomcat/pull/369


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] darmbrust commented on issue #5: ability to ignore file extensions?

[GitBox]

darmbrust commented on issue #5:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/5#issuecomment-705728238


   For reference, this is the zip file the tool is failing on:
   https://github.com/rubyzip/rubyzip/blob/master/test/data/gpbit3stored.zip
   
   From the context of the file its failing on, I'm guessing something isn't 
handling the general purpose bit flag properly when it is encountered:
   https://en.wikipedia.org/wiki/Zip_(file_format)#Local_file_header



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #369: UDecoder does not properly convert when EncodedSolidusHandling is PASS_THROUGH and non-solidus encoded characters are present before the sol

[GitBox]

markt-asf commented on pull request #369:
URL: https://github.com/apache/tomcat/pull/369#issuecomment-706113097


   Thanks for the PR. I've applied a variation of your proposed fix along with 
some additional unit tests to:
   - 10.0.x for 10.0.0-M10 onwards
   - 9.0.x for 9.0.40 onwards
   - 8.5.x for 8.5.60 onwards
   - 7.0.x for 7.0.107 onwards
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #369: UDecoder does not properly convert when EncodedSolidusHandling is PASS_THROUGH and non-solidus encoded characters are present before the sol

[GitBox]

markt-asf commented on pull request #369:
URL: https://github.com/apache/tomcat/pull/369#issuecomment-706113097


   Thanks for the PR. I've applied a variation of your proposed fix along with 
some additional unit tests to:
   - 10.0.x for 10.0.0-M10 onwards
   - 9.0.x for 9.0.40 onwards
   - 8.5.x for 8.5.60 onwards
   - 7.0.x for 7.0.107 onwards
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #369: UDecoder does not properly convert when EncodedSolidusHandling is PASS_THROUGH and non-solidus encoded characters are present before the solidus

[GitBox]

markt-asf closed pull request #369:
URL: https://github.com/apache/tomcat/pull/369


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] willmeck opened a new pull request #369: UDecoder does not properly convert when EncodedSolidusHandling is PASS_THROUGH and non-solidus encoded characters are present before the soli

[GitBox]

willmeck opened a new pull request #369:
URL: https://github.com/apache/tomcat/pull/369


   https://github.com/willmeck/tomcat-test-app - small test project showcasing 
the issue and fix
   
   When EncodedSolidusHandling is PASS_THROUGH, and other non-solidus encoded 
characters, such as encoded '+' (%2B) are present, the resultant string is 
incorrect. 
   Consider the string `abc%2Bde%2Ffg` 
   With PASS_THROUGH, it is expected that this would decode to `abc+de%2Ffg`
   However, looking at the UDecoder `convert()` code, this is what happens:
   
   buff contains `abc%2Bde%2Ffg`
   
   1. set idx to the first % character, `idx = 3`
   2. enter loop, setting j equal to idx, `idx=3` `j=3`
   3. buff[j] is `%` so enter else condition
   4. `j+=2` so `j=5 idx=3`
   5. set `res` equal to the char which is made up by j, j+1, and j+2
   6. res is not a `/`, so set buff[idx] = res (res is `+`) 
   
   buff now contains `abc+2Bde%2Ffg` and `j=5 idx=3`
   
   1. loop again so now `j=6 idx=4`
   2. buff[j] is `d`, so set buff[idx] = buff[j]
   3. loop again so now `j=7 idx=5'
   4. buff[j] is `e` so set buff[idx] = buff[j]
   
   buff now contains `abc+dede%2Ffg`
   
   1. loop again so now `j=8 idx=6`
   2. buff[j] is `%` so enter else condition
   3. `j+=2` so `j=10 idx=6`
   4. set `res` equal to the char which is made up by j, j+1, and j+2
   5. res is `/` so enter switch and go to case PASS_THROUGH
   6. `idx+=2` so `j=10 idx=8` (Note that at no point do you copy any chars 
from j to idx)
   
   buff now still contains `abc+dede%2Ffg`
   
   1. loop again so now `j=11 idx=9`
   2. buff[j] is `f` so set buff[idx] = buff[j]
   3. loop again so now `j=12 idx=10`
   4. buff[j] is `g` so set buff[idx] = buff[j]
   5. exit loop because reached length, adding 1 to idx, so `idx=11`
   6. set end at idx
   
   buff now contains `abc+dede%fg`
   
   The passthrough case statement expects j and idx to be equal, and that is 
not always the case, so we must explicitly copy those bytes. 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] darmbrust commented on issue #5: ability to ignore file extensions?

[GitBox]

darmbrust commented on issue #5:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/5#issuecomment-705728238


   For reference, this is the zip file the tool is failing on:
   https://github.com/rubyzip/rubyzip/blob/master/test/data/gpbit3stored.zip
   
   From the context of the file its failing on, I'm guessing something isn't 
handling the general purpose bit flag properly when it is encountered:
   https://en.wikipedia.org/wiki/Zip_(file_format)#Local_file_header



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] darmbrust opened a new issue #5: ability to ignore file extensions?

[GitBox]

darmbrust opened a new issue #5:
URL: https://github.com/apache/tomcat-jakartaee-migration/issues/5


   I've been using this tool, to try out an entire stack with these changes, 
and we have one webapp which is a JRuby application.
   
   Using this tool on the war file produced by our JRuby build, results in a 
failure:
   
   Migrating Jar entry [WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/]
   Migrating stream [WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/]
   Migrating Jar entry 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo.txt]
   Migrating stream [WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo.txt]
   Migrating Jar entry 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/bar/]
   Migrating stream 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/bar/]
   Migrating Jar entry 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/bar/baz/]
   Migrating stream 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/bar/baz/]
   Migrating Jar entry 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/bar/baz/foo.txt]
   Migrating stream 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/foo/bar/baz/foo.txt]
   Migrating Jar entry 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/food.txt]
   Migrating stream 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/globTest/food.txt]
   Migrating Jar entry 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/gpbit3stored.zip]
   Migrating archive 
[WEB-INF/gems/gems/rubyzip-1.2.2/test/data/gpbit3stored.zip]
   Error performing migration
   
   No additional details are given on the reason for the failure.
   Still looking into why the rubyzip gem has piles of test data in it... but 
that aside, it would be nice to tell this tool to ignore .txt and .zip files...



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #361: Add a GitHub action to lint the Markdown and YAML files.

[GitBox]

markt-asf commented on pull request #361:
URL: https://github.com/apache/tomcat/pull/361#issuecomment-703802985


   A couple of those files appear to be missing ALv2 headers. On that topic, it 
looks like the markdown files need an ALv2 header if possible as well.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #368: fix: CachedResource add path length

[GitBox]

markt-asf commented on pull request #368:
URL: https://github.com/apache/tomcat/pull/368#issuecomment-703797902


   Fixed in:
   - master for 10.0.0-M9 onwards
   - 9.0.x for 9.0.39 onwards
   - 8.5.x for 8.5.59 onwards
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #368: fix: CachedResource add path length

[GitBox]

markt-asf closed pull request #368:
URL: https://github.com/apache/tomcat/pull/368


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #368: fix: CachedResource add path length

[GitBox]

markt-asf commented on pull request #368:
URL: https://github.com/apache/tomcat/pull/368#issuecomment-703794569


   I've done some calculations and the absolute upper limit (the actual upper 
limit will be lower) is that a cache with a 10MB limit on size will actually 
use a little under 175MB. That isn't great but it isn't terrible either. I'll 
tweak the resource size calculation so it includes an approximate of the memory 
required to store the path.
   The proposed patch has a couple of issues:
   
   - It converts the string to bytes to determine the size.  This is both 
expensive and wrong. String length *2 +38 is a much closer approximation and I 
intend to ignore the 38 as that is already accounted for in the 500 byte 
overhead for each entry.
   - It includes the path size when determining if the resource exceeds the max 
object size (and hence won't have the content cached) but does not include the 
path size when performing the same check to determine when deciding whether to 
cache the content or not.
   
   I should have a fix for this committed shortly.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] markt-asf commented on issue #4: Download of Release

[GitBox]

markt-asf commented on issue #4:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/4#issuecomment-703474248


   We'd need to go through the ASF release process (not arduous but requires a 
little setting up as this is a new deliverable for the Tomcat project) and the 
primary release channel would need to be via the Tomcat website and the ASF 
mirror system. Additional channels, such as GitHub, could be added fairly 
easily if required.
   I can see this module being used as a component in other tools so Maven 
Central may make sense as well.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] martin-g commented on issue #4: Download of Release

[GitBox]

martin-g commented on issue #4:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/4#issuecomment-703465446


   Since this tool is not supposed to be used as a (Maven/Gradle) dependency 
there is no really point in uploading it to Maven Central.
   But we can publish GitHub releases - 
https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/managing-releases-in-a-repository.
   
   WDYT ?



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] Horcrux7 commented on issue #4: Download of Release

[GitBox]

Horcrux7 commented on issue #4:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/4#issuecomment-703237186


   A software developer has the source code of its projects and does not need 
your tool. It look for me more as a tool for administrators. So it might be.
   
   A tool of which the developers do not bother to spend time uploading a 
binary with the release (about a minute here on Github) does not seem to be of 
much value in the opinion of these developers. Then as a tester you don't need 
to bother to test it. Thanks for the clarification.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] ChristopherSchultz closed issue #4: Download of Release

[GitBox]

ChristopherSchultz closed issue #4:
URL: https://github.com/apache/tomcat-jakartaee-migration/issues/4


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] ChristopherSchultz commented on issue #4: Download of Release

[GitBox]

ChristopherSchultz commented on issue #4:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/4#issuecomment-703100871


   This is intended to be a tool used by developers. Anyone qualified to use 
this tool should also be qualified to build it from source. Don't expect a 
binary release anytime soon.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] Horcrux7 commented on issue #4: Download of Release

[GitBox]

Horcrux7 commented on issue #4:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/4#issuecomment-702938334


   Ok, I will wait until a binary version is available.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] martin-g commented on issue #4: Download of Release

[GitBox]

martin-g commented on issue #4:
URL: 
https://github.com/apache/tomcat-jakartaee-migration/issues/4#issuecomment-702628531


   There is no such at the moment. You have to build it locally.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-jakartaee-migration] Horcrux7 opened a new issue #4: Download of Release

[GitBox]

Horcrux7 opened a new issue #4:
URL: https://github.com/apache/tomcat-jakartaee-migration/issues/4


   Is there any download of the released version? For example in bintray or 
maven repository. I was not able to find it.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #360: Fix 64735 ServletContext.addJspFile() always fails with SecurityManager

[GitBox]

markt-asf closed pull request #360:
URL: https://github.com/apache/tomcat/pull/360


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf closed pull request #359: Fix 64735 ServletContext.addJspFile() always fails with SecurityManager

[GitBox]

markt-asf closed pull request #359:
URL: https://github.com/apache/tomcat/pull/359


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #359: Fix 64735 ServletContext.addJspFile() always fails with SecurityManager

[GitBox]

markt-asf commented on pull request #359:
URL: https://github.com/apache/tomcat/pull/359#issuecomment-701522483


   Thanks. Applied manually so I could make a few minor tweaks (mainly for 
consistency with existing tests). Also back-ported to 8.5.x and 7.0.x,



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #360: Fix 64735 ServletContext.addJspFile() always fails with SecurityManager

[GitBox]

markt-asf commented on pull request #360:
URL: https://github.com/apache/tomcat/pull/360#issuecomment-701522275


   Thanks. Applied manually so I could make a few minor tweaks (mainly for 
consistency with existing tests). Also back-ported to 8.5.x and 7.0.x,



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #359: Fix 64735 ServletContext.addJspFile() always fails with SecurityManager

[GitBox]

markt-asf commented on pull request #359:
URL: https://github.com/apache/tomcat/pull/359#issuecomment-701253890


   Many thanks for this. I particularly like the way you implemented the unit 
test. I am currently leaning towards packaging the test slightly differently so 
I'll apply this manually rather than just clicking merge.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] blueSky1825821 removed a comment on pull request #368: fix: CachedResource add path length

[GitBox]

blueSky1825821 removed a comment on pull request #368:
URL: https://github.com/apache/tomcat/pull/368#issuecomment-700584396


   > What is the justification for this change? What problem does it solve?
   
   When our application is loaded in another path or attacked in an unknown 
way, webapp path could take up a lot of space.
   org.apache.catalina.webresources.Cache#resourceCache can't be cleaned up, 
this happens in a production environment



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] blueSky1825821 commented on pull request #368: fix: CachedResource add path length

[GitBox]

blueSky1825821 commented on pull request #368:
URL: https://github.com/apache/tomcat/pull/368#issuecomment-700585098


   > > What is the justification for this change? What problem does it solve?
   > 
   > When our application is loaded in another path or attacked in an unknown 
way, webapp path could take up a lot of space(a few megabytes).
   > org.apache.catalina.webresources.Cache#resourceCache can't be cleaned up, 
this happens in a production environment
   
   
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] blueSky1825821 commented on pull request #368: fix: CachedResource add path length

[GitBox]

blueSky1825821 commented on pull request #368:
URL: https://github.com/apache/tomcat/pull/368#issuecomment-700584396


   > What is the justification for this change? What problem does it solve?
   
   When our application is loaded in another path or attacked in an unknown 
way, webapp path could take up a lot of space.
   org.apache.catalina.webresources.Cache#resourceCache can't be cleaned up, 
this happens in a production environment



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #368: fix: CachedResource add path length

[GitBox]

markt-asf commented on pull request #368:
URL: https://github.com/apache/tomcat/pull/368#issuecomment-700577634


   What is the justification for this change? What problem does it solve?



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] blueSky1825821 opened a new pull request #368: fix: CachedResource add path length

[GitBox]

blueSky1825821 opened a new pull request #368:
URL: https://github.com/apache/tomcat/pull/368


   cachedResource should add webapp path length



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kdillane commented on pull request #351: Remove White Spaces from the JSP files

[GitBox]

kdillane commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698458135


   > > Since there is no new test for this I am not sure but looking at the 
code I think "Remove white space" is not quite accurate.
   > > It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would 
match only if there zero or more empty spaces, followed by at least one new 
line, followed by 0 or more empty spaces.
   > > So the example above:
   > > ```
   > > | 
   > > | Quick Servlet Demo
   > > | 
   > > ```
   > > 
   > > 
   > > won't produce:
   > > ```
   > > |
   > > | Quick Servlet Demo
   > > | 
   > > ```
   > > 
   > > 
   > > In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`
   > 
   > I think the node text comes like this `\n` and this 
will be caught by the BLANK_LINE_PATTERN and thus will later be turned down to 
`\n` after the processing. I assume you are not taking the node correctly.
   > 
   > But on a broader aspect
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > Will produce
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > I agree to the minifying part, so we can rather just replace it with an 
empty character in the end instead of new line character and that will do the 
job too and will thus be more optimized.
   
   I'd recommend adding a test to cover this use-case.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kamnani commented on pull request #351: Remove White Spaces from the JSP files

[GitBox]

kamnani commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698430292


   > Since there is no new test for this I am not sure but looking at the code 
I think "Remove white space" is not quite accurate.
   > It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would match 
only if there zero or more empty spaces, followed by at least one new line, 
followed by 0 or more empty spaces.
   > So the example above:
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > won't produce:
   > 
   > ```
   > |
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`
   
   I think the node text comes like this `\n` and this will 
be caught by the BLANK_LINE_PATTERN and thus will later be turned down to 
`\n` after the processing. I assume you are not taking the node correctly.
   
   But on a broader aspect 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   Will produce 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kamnani edited a comment on pull request #351: Remove White Spaces from the JSP files

[GitBox]

kamnani edited a comment on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698430292


   > Since there is no new test for this I am not sure but looking at the code 
I think "Remove white space" is not quite accurate.
   > It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would match 
only if there zero or more empty spaces, followed by at least one new line, 
followed by 0 or more empty spaces.
   > So the example above:
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > won't produce:
   > 
   > ```
   > |
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`
   
   I think the node text comes like this `\n` and this will 
be caught by the BLANK_LINE_PATTERN and thus will later be turned down to 
`\n` after the processing. I assume you are not taking the node correctly.
   
   But on a broader aspect 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   Will produce 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   I agree to the minifying part, so we can rather just replace it with an 
empty character in the end instead of new line character and that will do the 
job too and will thus be more optimized. 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on pull request #351: Remove White Spaces from the JSP files

[GitBox]

martin-g commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698157237


   Since there is no new test for this I am not sure but looking at the code I 
think "Remove white space" is not quite accurate.
   It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would match 
only if there zero or more empty spaces, followed by at least one new line, 
followed by 0 or more empty spaces.
   So the example above:
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   won't produce:
   ```
   |
   | Quick Servlet Demo
   | 
   ```
   
   In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on a change in pull request #351: Remove White Spaces from the JSP files

[GitBox]

martin-g commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r494077383



##
File path: java/org/apache/jasper/compiler/NewlineReductionServletWriter.java
##
@@ -0,0 +1,41 @@
+package org.apache.jasper.compiler;
+
+import java.io.PrintWriter;
+
+/**
+ * This class filters duplicate newlines instructions from the compiler output,
+ * and therefore from the runtime JSP. The duplicates typically happen because
+ * the compiler has multiple branches that write them, but they operate
+ * independently and don't realize that the previous output was identical.
+ *
+ * Removing these lines makes the JSP more efficient by executing fewer 
operations during runtime.
+ *
+ * @author Engebretson, John
+ * @author Kamnani, Jatin
+ *
+ */
+public class NewlineReductionServletWriter extends ServletWriter {
+private static final String NEWLINE_WRITE_TEXT = "out.write('\\n');";

Review comment:
   This one is a bit strange.
   I haven't written JSP code in more than 10 years now but if I want to 
produce several new lines I'd do `out.write("\n\n\n\n")` instead of 4 times 
`out.write('\\n');`.
   In what cases one would have several `out.write('\\n');` ?

##
File path: java/org/apache/jasper/compiler/Generator.java
##
@@ -2095,6 +2101,20 @@ public void visit(Node.JspElement n) throws 
JasperException {
 public void visit(Node.TemplateText n) throws JasperException {
 
 String text = n.getText();
+// If the flag is active, attempt to minimize the frequency of
+// regex operations.
+if ((ctxt!=null) &&
+ctxt.getOptions().getJSPWhiteSpaceTrimFlag() &&
+text.contains("\n")) {
+// Ensure there are no  or  tags embedded in this
+// text - if there are, we want to NOT modify the whitespace.
+Matcher preMatcher = PRE_TAG_PATTERN.matcher(text);
+if (!preMatcher.matches()) {
+Matcher matcher = BLANK_LINE_PATTERN.matcher(text);
+String revisedText = matcher.replaceAll("\n");

Review comment:
   This will drop also any `\r`s in the string. Is this intentional ?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

[GitBox]

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r494418331



##
File path: java/org/apache/jasper/compiler/Generator.java
##
@@ -2095,6 +2101,20 @@ public void visit(Node.JspElement n) throws 
JasperException {
 public void visit(Node.TemplateText n) throws JasperException {
 
 String text = n.getText();
+// If the flag is active, attempt to minimize the frequency of
+// regex operations.
+if ((ctxt!=null) &&
+ctxt.getOptions().getJSPWhiteSpaceTrimFlag() &&
+text.contains("\n")) {
+// Ensure there are no  or  tags embedded in this
+// text - if there are, we want to NOT modify the whitespace.
+Matcher preMatcher = PRE_TAG_PATTERN.matcher(text);
+if (!preMatcher.matches()) {
+Matcher matcher = BLANK_LINE_PATTERN.matcher(text);
+String revisedText = matcher.replaceAll("\n");

Review comment:
   yes, it is intentional. We replace it with just one "\n. 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kdillane commented on pull request #351: Remove White Spaces from the JSP files

[GitBox]

kdillane commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698458135


   > > Since there is no new test for this I am not sure but looking at the 
code I think "Remove white space" is not quite accurate.
   > > It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would 
match only if there zero or more empty spaces, followed by at least one new 
line, followed by 0 or more empty spaces.
   > > So the example above:
   > > ```
   > > | 
   > > | Quick Servlet Demo
   > > | 
   > > ```
   > > 
   > > 
   > > won't produce:
   > > ```
   > > |
   > > | Quick Servlet Demo
   > > | 
   > > ```
   > > 
   > > 
   > > In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`
   > 
   > I think the node text comes like this `\n` and this 
will be caught by the BLANK_LINE_PATTERN and thus will later be turned down to 
`\n` after the processing. I assume you are not taking the node correctly.
   > 
   > But on a broader aspect
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > Will produce
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > I agree to the minifying part, so we can rather just replace it with an 
empty character in the end instead of new line character and that will do the 
job too and will thus be more optimized.
   
   I'd recommend adding a test to cover this use-case.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kamnani edited a comment on pull request #351: Remove White Spaces from the JSP files

[GitBox]

kamnani edited a comment on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698430292


   > Since there is no new test for this I am not sure but looking at the code 
I think "Remove white space" is not quite accurate.
   > It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would match 
only if there zero or more empty spaces, followed by at least one new line, 
followed by 0 or more empty spaces.
   > So the example above:
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > won't produce:
   > 
   > ```
   > |
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`
   
   I think the node text comes like this `\n` and this will 
be caught by the BLANK_LINE_PATTERN and thus will later be turned down to 
`\n` after the processing. I assume you are not taking the node correctly.
   
   But on a broader aspect 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   Will produce 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   I agree to the minifying part, so we can rather just replace it with an 
empty character in the end instead of new line character and that will do the 
job too and will thus be more optimized. 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kamnani commented on pull request #351: Remove White Spaces from the JSP files

[GitBox]

kamnani commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698430292


   > Since there is no new test for this I am not sure but looking at the code 
I think "Remove white space" is not quite accurate.
   > It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would match 
only if there zero or more empty spaces, followed by at least one new line, 
followed by 0 or more empty spaces.
   > So the example above:
   > 
   > ```
   > | 
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > won't produce:
   > 
   > ```
   > |
   > | Quick Servlet Demo
   > | 
   > ```
   > 
   > In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`
   
   I think the node text comes like this `\n` and this will 
be caught by the BLANK_LINE_PATTERN and thus will later be turned down to 
`\n` after the processing. I assume you are not taking the node correctly.
   
   But on a broader aspect 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   Will produce 
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] kamnani commented on a change in pull request #351: Remove White Spaces from the JSP files

[GitBox]

kamnani commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r494418331



##
File path: java/org/apache/jasper/compiler/Generator.java
##
@@ -2095,6 +2101,20 @@ public void visit(Node.JspElement n) throws 
JasperException {
 public void visit(Node.TemplateText n) throws JasperException {
 
 String text = n.getText();
+// If the flag is active, attempt to minimize the frequency of
+// regex operations.
+if ((ctxt!=null) &&
+ctxt.getOptions().getJSPWhiteSpaceTrimFlag() &&
+text.contains("\n")) {
+// Ensure there are no  or  tags embedded in this
+// text - if there are, we want to NOT modify the whitespace.
+Matcher preMatcher = PRE_TAG_PATTERN.matcher(text);
+if (!preMatcher.matches()) {
+Matcher matcher = BLANK_LINE_PATTERN.matcher(text);
+String revisedText = matcher.replaceAll("\n");

Review comment:
   yes, it is intentional. We replace it with just one "\n. 





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on pull request #351: Remove White Spaces from the JSP files

[GitBox]

martin-g commented on pull request #351:
URL: https://github.com/apache/tomcat/pull/351#issuecomment-698157237


   Since there is no new test for this I am not sure but looking at the code I 
think "Remove white space" is not quite accurate.
   It is rather "Remove empty lines". Because BLANK_LINE_PATTERN would match 
only if there zero or more empty spaces, followed by at least one new line, 
followed by 0 or more empty spaces.
   So the example above:
   
   ```
   | 
   | Quick Servlet Demo
   | 
   ```
   
   won't produce:
   ```
   |
   | Quick Servlet Demo
   | 
   ```
   
   In addition: why preserve 1 new line ? HTML minifiers would produce 
something like `.`



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat] martin-g commented on a change in pull request #351: Remove White Spaces from the JSP files

[GitBox]

martin-g commented on a change in pull request #351:
URL: https://github.com/apache/tomcat/pull/351#discussion_r494079321



##
File path: java/org/apache/jasper/compiler/Generator.java
##
@@ -2095,6 +2101,20 @@ public void visit(Node.JspElement n) throws 
JasperException {
 public void visit(Node.TemplateText n) throws JasperException {
 
 String text = n.getText();
+// If the flag is active, attempt to minimize the frequency of
+// regex operations.
+if ((ctxt!=null) &&
+ctxt.getOptions().getJSPWhiteSpaceTrimFlag() &&
+text.contains("\n")) {
+// Ensure there are no  or  tags embedded in this
+// text - if there are, we want to NOT modify the whitespace.
+Matcher preMatcher = PRE_TAG_PATTERN.matcher(text);
+if (!preMatcher.matches()) {
+Matcher matcher = BLANK_LINE_PATTERN.matcher(text);
+String revisedText = matcher.replaceAll("\n");

Review comment:
   This will drop also any `\r`s in the string. Is this intentional ?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org