[GitHub] [tomcat] markt-asf commented on a diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094835289
##
java/org/apache/tomcat/buildutil/translate/Utils.java:
##
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("$1");
-result = result.replaceAll("\t", "\\t");
+result = result.replace("\t", "t");
Review Comment:
This is not correct. The literal (unescaped) replacement text should be `\t`
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] tbw777 commented on a diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
tbw777 commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094863229
##
java/org/apache/tomcat/buildutil/translate/Utils.java:
##
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("$1");
-result = result.replaceAll("\t", "\\t");
+result = result.replace("\t", "t");
Review Comment:
The second argument in replaceAll means escape t, not \t.
You can check proofs tests. equals returning true if replace maked to t, not
\t
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094884701
##
java/org/apache/tomcat/buildutil/translate/Utils.java:
##
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("$1");
-result = result.replaceAll("\t", "\\t");
+result = result.replace("\t", "t");
Review Comment:
It is a bug in the original code. It should be replacing all tabs with the
literal string `\t`.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094893763
##
java/org/apache/tomcat/buildutil/translate/Utils.java:
##
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("$1");
-result = result.replaceAll("\t", "\\t");
+result = result.replace("\t", "t");
Review Comment:
For the record, none of the current i18n strings use tabs. I did find some
messages using them in early 7.0.x versions, but those messages were never
referenced in the Java code.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] tbw777 commented on pull request #581: Speedup by removing non pattern replaceAll with constant arg
tbw777 commented on PR #581: URL: https://github.com/apache/tomcat/pull/581#issuecomment-1414189316 @markt-asf Branch was updated and https://gist.github.com/tbw777/8a6ef60af21487c5faec67037099fd0b also Check please. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] gksxodnd007 commented on pull request #579: Add activateDropConnection config to choose drop connection by http status code
gksxodnd007 commented on PR #579: URL: https://github.com/apache/tomcat/pull/579#issuecomment-1414693058 @markt-asf I see the reactor netty code to check how they close the connection when the current request was not fully read. they also close the connection even though that is keep-alive, when the decode processing of the request failed. how about adding defensive code to prevent the request smuggling risks? if I add the code, would you reconsider about dropping the connection by HTTP status code? ref: - https://github.com/reactor/reactor-netty/blob/c71ebe4372f35496eb04471355ea84739bc6381a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpTrafficHandler.java#L193-L197 - https://github.com/reactor/reactor-netty/blob/c71ebe4372f35496eb04471355ea84739bc6381a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpTrafficHandler.java#L280-L283 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #579: Add activateDropConnection config to choose drop connection by http status code
rmaucher commented on PR #579: URL: https://github.com/apache/tomcat/pull/579#issuecomment-1415375850 Of course, some instances of these status codes would be "ok" to recover from, but Mark is actually right: it is not going to be deterministic. Also it is not possible to add "defensive" code since starting to decode random bytes in a random location will allow request smuggling (a proxy and Tomcat have to both "see" the same HTTP requests coming in and process them the same way). If you are using Tomcat without proxying of any kind (that happens very often, right ?), then it is less dangerous although there is some DoS potential on HTTP header parsing errors. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1095639347
##
java/org/apache/tomcat/buildutil/translate/Utils.java:
##
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("$1");
-result = result.replaceAll("\t", "\\t");
+result = result.replace("\t", "t");
Review Comment:
Tx.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 merged pull request #581: Speedup by removing non pattern replaceAll with constant arg
markt-asf merged PR #581: URL: https://github.com/apache/tomcat/pull/581 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #581: Speedup by removing non pattern replaceAll with constant arg
ChristopherSchultz commented on PR #581: URL: https://github.com/apache/tomcat/pull/581#issuecomment-1416015385 There isn't much of a difference between `String.replace` and `String.replaceAll`. They both use regular expressions under the hood. Java's `Pattern` class has a special case for "this search-string doesn't include any metacharacters" (it's a flag called `LITERAL`) and has optimized code-paths for those cases. IMO `replaceAll` is more clear than `replace` because it's absolutely clear that all instances will be replaced in the string. If performance is the primary factor, here, we should use an implementation of string-replacement that does not involve creating `Pattern` and `Matcher` objects and re-encoding the replacement String _every single time_. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #581: Speedup by removing non pattern replaceAll with constant arg
ChristopherSchultz commented on PR #581: URL: https://github.com/apache/tomcat/pull/581#issuecomment-1416017516 I think this "performance improvement" is not an improvement and doesn't affect performance in any meaningful way. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #581: Speedup by removing non pattern replaceAll with constant arg
ChristopherSchultz commented on PR #581: URL: https://github.com/apache/tomcat/pull/581#issuecomment-1416024123 Apologies: my comments are rubbish after Java 9. Please forgive the noise. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] gksxodnd007 commented on pull request #579: Add activateDropConnection config to choose drop connection by http status code
gksxodnd007 commented on PR #579: URL: https://github.com/apache/tomcat/pull/579#issuecomment-1416087598 @rmaucher thank you for your explanation :) I understand. is there any better way that reuses the connection for utilizing computing resources? -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #579: Add activateDropConnection config to choose drop connection by http status code
markt-asf commented on PR #579: URL: https://github.com/apache/tomcat/pull/579#issuecomment-1416093124 My suggestion at this point would be to use custom 4xx and 5xx status codes in the application rather than re-using known codes. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] gksxodnd007 commented on pull request #579: Add activateDropConnection config to choose drop connection by http status code
gksxodnd007 commented on PR #579: URL: https://github.com/apache/tomcat/pull/579#issuecomment-1416100562 @markt-asf I'm sorry, but I'll follow your opinion. and I learn about the request smuggling risks while communication with you. thank you very much :) -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] gksxodnd007 closed pull request #579: Add activateDropConnection config to choose drop connection by http status code
gksxodnd007 closed pull request #579: Add activateDropConnection config to choose drop connection by http status code URL: https://github.com/apache/tomcat/pull/579 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 opened a new pull request, #583: Replaced old time API
tbw777 opened a new pull request, #583: URL: https://github.com/apache/tomcat/pull/583 SimpleDateFormatter isnt thread safe and deprecated with new jvm8 time API -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #579: Add activateDropConnection config to choose drop connection by http status code
markt-asf commented on PR #579: URL: https://github.com/apache/tomcat/pull/579#issuecomment-1420408534 No need for an apology. This has been a useful discussion. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan merged pull request #584: Use filtersets for IDE config libs and versioning
aooohan merged PR #584: URL: https://github.com/apache/tomcat/pull/584 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan commented on pull request #584: Use filtersets for IDE config libs and versioning
aooohan commented on PR #584: URL: https://github.com/apache/tomcat/pull/584#issuecomment-1423869152 Thanks, I've cherry-picked this commit to other branch and make some minor modification. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #506: Adding Redirect and Proxy Error Reporting Valves
rmaucher commented on PR #506: URL: https://github.com/apache/tomcat/pull/506#issuecomment-1424311002 I had a look at it and ended up merging the feature as https://github.com/apache/tomcat/commit/2c2a1bd248b1ee200a0898c30e355802f0a6f7a6 We'll see what the final feedback on it is ... -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #506: Adding Redirect and Proxy Error Reporting Valves
rmaucher closed pull request #506: Adding Redirect and Proxy Error Reporting Valves URL: https://github.com/apache/tomcat/pull/506 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] Madfarm opened a new pull request, #585: Update README.md
Madfarm opened a new pull request, #585: URL: https://github.com/apache/tomcat/pull/585 You can deny this; I had to create a pull request for a school project -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan closed pull request #585: Update README.md
aooohan closed pull request #585: Update README.md URL: https://github.com/apache/tomcat/pull/585 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan commented on pull request #587: Removed unused assignments
aooohan commented on PR #587: URL: https://github.com/apache/tomcat/pull/587#issuecomment-1425410871 -1 I don't think this change makes sense to do that, the current code is a bit more intuitive. As a reminder, when you propose a PR, we hope you will add some description to avoid wasting your time and ours. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan closed pull request #587: Removed unused assignments
aooohan closed pull request #587: Removed unused assignments URL: https://github.com/apache/tomcat/pull/587 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan merged pull request #588: Removed object creation
aooohan merged PR #588: URL: https://github.com/apache/tomcat/pull/588 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan commented on a diff in pull request #590: Closing ExecutorService with try-with-resources and other
aooohan commented on code in PR #590:
URL: https://github.com/apache/tomcat/pull/590#discussion_r1102455984
##
java/org/apache/tomcat/util/http/fileupload/util/Streams.java:
##
@@ -106,12 +106,12 @@ public static long copy(final InputStream inputStream,
}
if (out != null) {
if (closeOutputStream) {
-out.close();
+out.close(); //fixme will closing always with try
Review Comment:
This package(`org.apache.tomcat.util.http.fileupload`) is internal fork of
Commons File Upload. Please do not modify any of the code in this.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] aooohan merged pull request #590: Closing ExecutorService with try-with-resources and other
aooohan merged PR #590: URL: https://github.com/apache/tomcat/pull/590 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] dsoumis opened a new pull request, #16: Update build.xml with new year
dsoumis opened a new pull request, #16: URL: https://github.com/apache/tomcat-native/pull/16 Update build.xml with new year. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] markt-asf closed pull request #16: Update build.xml with new year
markt-asf closed pull request #16: Update build.xml with new year URL: https://github.com/apache/tomcat-native/pull/16 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] markt-asf commented on pull request #16: Update build.xml with new year
markt-asf commented on PR #16: URL: https://github.com/apache/tomcat-native/pull/16#issuecomment-1425572722 Thanks for spotting this. Better to automate this as I'll just forget to update it again next year. I'll copy the config to do this from the Tomcat 11.0.x build. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 commented on pull request #590: Closing ExecutorService with try-with-resources and other
tbw777 commented on PR #590:
URL: https://github.com/apache/tomcat/pull/590#issuecomment-1426250991
@aooohan
```
ExecutorService
...
Since:
19
...
default void close() {
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] aooohan commented on pull request #589: XSD patterns normalization
aooohan commented on PR #589: URL: https://github.com/apache/tomcat/pull/589#issuecomment-1427559632 We are not able to modify these files, maybe you should report this to [Jakarta project](https://github.com/jakartaee). -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan closed pull request #589: XSD patterns normalization
aooohan closed pull request #589: XSD patterns normalization URL: https://github.com/apache/tomcat/pull/589 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 commented on pull request #589: XSD patterns normalization
tbw777 commented on PR #589: URL: https://github.com/apache/tomcat/pull/589#issuecomment-1427636757 @aooohan ty -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #586: Fixed closing serverSocket in doCloseServerSocket()
markt-asf commented on PR #586: URL: https://github.com/apache/tomcat/pull/586#issuecomment-1429675397 Thanks for the report. Fixed slightly differently (effectively the same code) to align with the existing NIO2 code. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #586: Fixed closing serverSocket in doCloseServerSocket()
markt-asf closed pull request #586: Fixed closing serverSocket in doCloseServerSocket() URL: https://github.com/apache/tomcat/pull/586 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 diff in pull request #583: Replaced old time API
markt-asf commented on code in PR #583:
URL: https://github.com/apache/tomcat/pull/583#discussion_r110573
##
java/org/apache/catalina/filters/RequestDumperFilter.java:
##
@@ -56,12 +59,13 @@ public class RequestDumperFilter extends GenericFilter {
private static final String NON_HTTP_REQ_MSG = "Not available. Non-http
request.";
private static final String NON_HTTP_RES_MSG = "Not available. Non-http
response.";
-private static final ThreadLocal timestamp =
ThreadLocal.withInitial(Timestamp::new);
-
// Log must be non-static as loggers are created per class-loader and this
// Filter may be used in multiple class loaders
private transient Log log = LogFactory.getLog(RequestDumperFilter.class);
+private static final ZoneId ZONE_ID = ZoneId.systemDefault();
+private static final DateTimeFormatter DF_DATETIME =
DateTimeFormatter.ofPattern("dd-MM- HH:mm:ss");
+private static final AtomicLong ATOMIC_LONG = new AtomicLong();
Review Comment:
Need better names - at least for the AtomicLong
##
java/org/apache/catalina/filters/RequestDumperFilter.java:
##
@@ -239,17 +243,11 @@ private void doLog(String attribute, String value) {
}
private String getTimestamp() {
-Timestamp ts = timestamp.get();
long currentTime = System.currentTimeMillis();
-
-if ((ts.date.getTime() + 999) < currentTime) {
-ts.date.setTime(currentTime - (currentTime % 1000));
-ts.update();
-}
-return ts.dateString;
+long res = ATOMIC_LONG.updateAndGet(it -> it + 999 < currentTime ?
currentTime - (currentTime % 1000) : it);
Review Comment:
This appears to create a source of contention across threads - exactly what
the current code is designed to avoid.
##
java/org/apache/catalina/manager/JspHelper.java:
##
@@ -72,14 +74,19 @@ public static String guessDisplayUserFromSession(Session
in_session) {
return escapeXml(user);
}
+private static String getDateTimeStr(long value) {
+Instant instant = Instant.ofEpochMilli(value);
+LocalDateTime ldt = LocalDateTime.ofInstant(instant, ZONE_ID);
+return DATE_TIME_FORMAT.format(ldt);
+}
Review Comment:
It is different. But why is it better?
##
java/org/apache/catalina/util/Strftime.java:
##
@@ -113,36 +113,52 @@ public class Strftime {
*/
public Strftime( String origFormat, Locale locale ) {
String convertedFormat = convertDateFormat( origFormat );
-simpleDateFormat = new SimpleDateFormat( convertedFormat, locale );
+dtf = DateTimeFormatter.ofPattern(convertedFormat).withLocale(locale);
}
/**
* Format the date according to the strftime-style string given in the
constructor.
*
- * @param date the date to format
+ * @param instant the date to format
* @return the formatted date
*/
-public String format( Date date ) {
-return simpleDateFormat.format( date );
+public String format(Instant instant) {
Review Comment:
Changes to public APIs need more work. Generally, the old API is deprecated
in the latest stable release (and possible older releases) and the new API is
added to at least the current development release. The new API may be
back-ported to older releases where it makes sense to do so.
##
java/org/apache/tomcat/dbcp/pool2/impl/DefaultPooledObjectInfo.java:
##
@@ -30,7 +33,7 @@
*/
public class DefaultPooledObjectInfo implements DefaultPooledObjectInfoMBean {
-private static final String PATTERN = "-MM-dd HH:mm:ss Z";
+private static final DateTimeFormatter DTF =
DateTimeFormatter.ofPattern("-MM-dd HH:mm:ss Z");
private final PooledObject pooledObject;
Review Comment:
This code is forked from Commons Pool. Changes need to be made there.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 #582: Enabled headless build configuration
markt-asf commented on PR #582: URL: https://github.com/apache/tomcat/pull/582#issuecomment-1429706205 What problem is this PR trying to 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 commented on pull request #582: Enabled headless build configuration
tbw777 commented on PR #582: URL: https://github.com/apache/tomcat/pull/582#issuecomment-1429719724 no problem, just selecting less jvm api to work -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #582: Enabled headless build configuration
ChristopherSchultz commented on PR #582: URL: https://github.com/apache/tomcat/pull/582#issuecomment-1429782733 If no code requests anything AWT-related, then the headless config of the JVM makes no difference. I see no improvement here at all, just added lines to an already complex build script. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 commented on pull request #582: Enabled headless build configuration
tbw777 commented on PR #582: URL: https://github.com/apache/tomcat/pull/582#issuecomment-1429800793 Also this is guarantee that no awt components used -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #578: Remove redundant modifiers for interface members
markt-asf commented on PR #578: URL: https://github.com/apache/tomcat/pull/578#issuecomment-1430113794 I've applied this change via a combination of IDE tools and manual review as the CheckStyle test for redundant modifiers covers more than just interface methods. I was initially in two minds whether to apply this but as I worked through it, I could see the benefits it offers in making the visibility - particularly of constructors - clearer. Prior to this patch, many were marked as public even though there were effectively package private. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #578: Remove redundant modifiers for interface members
markt-asf closed pull request #578: Remove redundant modifiers for interface members URL: https://github.com/apache/tomcat/pull/578 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #582: Enabled headless build configuration
ChristopherSchultz commented on PR #582: URL: https://github.com/apache/tomcat/pull/582#issuecomment-1430168053 It does not guarantee that no AWT components are used. It only guarantees that anything which requires an actual screen will fail (such as trying to open a Frame, etc.). AWT is still perfectly usable when "headless". In fact, that's the whole point of putting the JVM into "headless" mode: to allow AWT to be used even when there isn't a "head" (screen). -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #582: Enabled headless build configuration
markt-asf commented on PR #582: URL: https://github.com/apache/tomcat/pull/582#issuecomment-1430228290 I don't see any benefit to adding these extra settings. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #582: Enabled headless build configuration
markt-asf closed pull request #582: Enabled headless build configuration URL: https://github.com/apache/tomcat/pull/582 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] dsoumis opened a new pull request, #591: Implement more special maps for rewrite valve
dsoumis opened a new pull request, #591: URL: https://github.com/apache/tomcat/pull/591 Implemented more special maps from https://httpd.apache.org/docs/2.4/rewrite/rewritemap.html as implied by the FIXME tag. Added relevant tests and added ServletMapping to test int: MapType. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 closed pull request #583: Replaced old time API
tbw777 closed pull request #583: Replaced old time API URL: https://github.com/apache/tomcat/pull/583 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 opened a new pull request, #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
tbw777 opened a new pull request, #592: URL: https://github.com/apache/tomcat/pull/592 https://gist.github.com/tbw777/8ce56d2cc3a0216012362d18b7262eb2 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] zengwei2000 opened a new pull request, #593: Update CONTRIBUTING.md
zengwei2000 opened a new pull request, #593: URL: https://github.com/apache/tomcat/pull/593 fix typo -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #593: Update CONTRIBUTING.md
markt-asf closed pull request #593: Update CONTRIBUTING.md URL: https://github.com/apache/tomcat/pull/593 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #593: Update CONTRIBUTING.md
markt-asf commented on PR #593: URL: https://github.com/apache/tomcat/pull/593#issuecomment-1437964475 That spelling is correct for US English. Given the range of contributors to Tomcat, there is a mix of UK and US English used in the Tomcat docs. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #591: Implement more special maps for rewrite valve
rmaucher commented on PR #591: URL: https://github.com/apache/tomcat/pull/591#issuecomment-1440111376 I merged it with a lot of changes and cleanups. Thanks. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #591: Implement more special maps for rewrite valve
rmaucher closed pull request #591: Implement more special maps for rewrite valve URL: https://github.com/apache/tomcat/pull/591 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 diff in pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
markt-asf commented on code in PR #592: URL: https://github.com/apache/tomcat/pull/592#discussion_r1116803461 ## java/jakarta/servlet/jsp/resources/jspxml.xsd: ## @@ -25,7 +25,7 @@ - + Review Comment: The Tomcat project is unable to change these schema. We have to use the ones provided by the specification. Please direct this part of the PR to the Jakarta Schema project: https://github.com/eclipse-ee4j/jakartaee-schemas -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
ChristopherSchultz commented on PR #592: URL: https://github.com/apache/tomcat/pull/592#issuecomment-1443631640 I'm curious about performance data when the `\w` has other things added to it, which is the case for all examples in Tomcat. This microbenchmark only compares `[A-Za-z0-9_]` as the whole character class against `\w` and `[\w]` without any other characters added to the character class. Also, most expressions used in Tomcat have a trailing `+` which in the attached performance data show that performance is again terrible. It still appears measurably and consistently better than `[A-Za-z0-9_]` but I think it's important to benchmark what Tomcat _actually_ uses and not something _very close_ to what Tomcat uses. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 commented on pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
tbw777 commented on PR #592: URL: https://github.com/apache/tomcat/pull/592#issuecomment-1444501056 https://gist.github.com/tbw777/cd394ec67a01f9e7e8fe4d0c66d74637 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
ChristopherSchultz commented on PR #592: URL: https://github.com/apache/tomcat/pull/592#issuecomment-1444608082 Thanks @tbw777 for the updated micro-benchmarks. I agree that the performance improvement is significant enough to warrant a change to Tomcat. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] tbw777 commented on pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
tbw777 commented on PR #592: URL: https://github.com/apache/tomcat/pull/592#issuecomment-1445017914 Compile speed https://gist.github.com/tbw777/6a6303f64894e65a160d3f6321685d27 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] sharmanalin59 opened a new issue, #43: The AJP Connector is configured with secretRequired="true"
sharmanalin59 opened a new issue, #43: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/43 Kindly help with the below issue-: 25-Feb-2023 00:18:50.221 SEVERE [main] org.apache.catalina.util.LifecycleBase.handleSubClassException Failed to start component [Connector[AJP/1.3-7009]] org.apache.catalina.LifecycleException: Protocol handler start failed at org.apache.catalina.connector.Connector.startInternal(Connector.java:1076) at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:183) at org.apache.catalina.core.StandardService.startInternal(StandardService.java:447) at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:183) at org.apache.catalina.core.StandardServer.startInternal(StandardServer.java:930) at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:183) at org.apache.catalina.startup.Catalina.start(Catalina.java:772) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.apache.catalina.startup.Bootstrap.start(Bootstrap.java:347) at org.apache.catalina.startup.Bootstrap.main(Bootstrap.java:478) Caused by: java.lang.IllegalArgumentException: The AJP Connector is configured with secretRequired="true" but the secret attribute is either null or "". This combination is not valid. at org.apache.coyote.ajp.AbstractAjpProtocol.start(AbstractAjpProtocol.java:267) at org.apache.catalina.connector.Connector.startInternal(Connector.java:1073) ... 12 more 25-Feb-2023 00:18:50.223 INFO [main] org.apache.catalina.startup.Catalina.start Server startup in [1048] milliseconds (END) Connection to 10.252.4.140 closed by remote host. Spring boot version - 2.0.0.RELEASE and tomcat 9.0.70, maven 3.6.x, java 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org.apache.org 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 #43: The AJP Connector is configured with secretRequired="true"
markt-asf commented on issue #43: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/43#issuecomment-1445967136 Wrong project, wrong forum. You want the Tomcat users mailing list: https://tomcat.apache.org/lists.html#tomcat-users -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 closed issue #43: The AJP Connector is configured with secretRequired="true"
markt-asf closed issue #43: The AJP Connector is configured with secretRequired="true" URL: https://github.com/apache/tomcat-jakartaee-migration/issues/43 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] sharmanalin59 commented on issue #43: The AJP Connector is configured with secretRequired="true"
sharmanalin59 commented on issue #43: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/43#issuecomment-1445973344 Which is the right forum for tomcat -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #43: The AJP Connector is configured with secretRequired="true"
markt-asf commented on issue #43: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/43#issuecomment-1446007408 Please read my previous comment. This is the issue tracker for the Tomcat migration tool for Jakarta EE, not Tomcat. Tomcat questions should be directed to the mailing list. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] sharmanalin59 commented on issue #43: The AJP Connector is configured with secretRequired="true"
sharmanalin59 commented on issue #43: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/43#issuecomment-1446063343 what is mailing list? -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #43: The AJP Connector is configured with secretRequired="true"
markt-asf commented on issue #43: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/43#issuecomment-1446069013 https://tomcat.apache.org/lists.html#tomcat-users -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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-training] dependabot[bot] opened a new pull request, #13: Bump minimist from 0.0.8 to 1.2.8
dependabot[bot] opened a new pull request, #13: URL: https://github.com/apache/tomcat-training/pull/13 Bumps [minimist](https://github.com/minimistjs/minimist) from 0.0.8 to 1.2.8. Changelog Sourced from https://github.com/minimistjs/minimist/blob/main/CHANGELOG.md";>minimist's changelog. https://github.com/minimistjs/minimist/compare/v1.2.7...v1.2.8";>v1.2.8 - 2023-02-09 Merged [Fix] Fix long option followed by single dash https://github-redirect.dependabot.com/minimistjs/minimist/pull/17";>[#17](https://github.com/minimistjs/minimist/issues/17) [Tests] Remove duplicate test https://github-redirect.dependabot.com/minimistjs/minimist/pull/12";>[#12](https://github.com/minimistjs/minimist/issues/12) [Fix] opt.string works with multiple aliases https://github-redirect.dependabot.com/minimistjs/minimist/pull/10";>[#10](https://github.com/minimistjs/minimist/issues/10) Fixed [Fix] Fix long option followed by single dash (https://github-redirect.dependabot.com/minimistjs/minimist/issues/17";>#17) https://github-redirect.dependabot.com/minimistjs/minimist/issues/15";>[#15](https://github.com/minimistjs/minimist/issues/15) [Tests] Remove duplicate test (https://github-redirect.dependabot.com/minimistjs/minimist/issues/12";>#12) https://github-redirect.dependabot.com/minimistjs/minimist/issues/8";>[#8](https://github.com/minimistjs/minimist/issues/8) [Fix] Fix long option followed by single dash https://github-redirect.dependabot.com/minimistjs/minimist/issues/15";>[#15](https://github.com/minimistjs/minimist/issues/15) [Fix] opt.string works with multiple aliases (https://github-redirect.dependabot.com/minimistjs/minimist/issues/10";>#10) https://github-redirect.dependabot.com/minimistjs/minimist/issues/9";>[#9](https://github.com/minimistjs/minimist/issues/9) [Fix] Fix handling of short option with non-trivial equals https://github-redirect.dependabot.com/minimistjs/minimist/issues/5";>[#5](https://github.com/minimistjs/minimist/issues/5) [Tests] Remove duplicate test https://github-redirect.dependabot.com/minimistjs/minimist/issues/8";>[#8](https://github.com/minimistjs/minimist/issues/8) [Fix] opt.string works with multiple aliases https://github-redirect.dependabot.com/minimistjs/minimist/issues/9";>[#9](https://github.com/minimistjs/minimist/issues/9) Commits Merge tag 'v0.2.3' https://github.com/minimistjs/minimist/commit/a0267947c7870fc5847cf2d437fbe33f392767da";>a026794 [eslint] fix indentation and whitespace https://github.com/minimistjs/minimist/commit/5368ca4147e974138a54cc0dc4cea8f756546b70";>5368ca4 [eslint] fix indentation and whitespace https://github.com/minimistjs/minimist/commit/e5f5067259ceeaf0b098d14bec910f87e58708c7";>e5f5067 [eslint] more cleanup https://github.com/minimistjs/minimist/commit/62fde7d935f83417fb046741531a9e2346a36976";>62fde7d [eslint] more cleanup https://github.com/minimistjs/minimist/commit/36ac5d0d95e4947d074e5737d94814034ca335d1";>36ac5d0 [meta] add auto-changelog https://github.com/minimistjs/minimist/commit/73923d223553fca08b1ba77e3fbc2a492862ae4c";>73923d2 [actions] add reusable workflows https://github.com/minimistjs/minimist/commit/d80727df77bfa9e631044d7f16368d8f09242c91";>d80727d [eslint] add eslint; rules to enable later are warnings https://github.com/minimistjs/minimist/commit/48bc06a1b41f00e9cdf183db34f7a51ba70e98d4";>48bc06a [eslint] fix indentation https://github.com/minimistjs/minimist/commit/34b0f1ccaa45183c3c4f06a91f9b405180a6f982";>34b0f1c [readme] rename and add badges https://github.com/minimistjs/minimist/commit/5df0fe49211bd09a3636f8686a7cb3012c3e98f0";>5df0fe4 [Dev Deps] switch from covert to nyc https://github.com/minimistjs/minimist/commit/a48b128fdb8d427dfb20a15273f83e38d97bef07";>a48b128 [Dev Deps] update covert, tape; remove unnecessary tap https://github.com/minimistjs/minimist/commit/f0fb958e9a1fe980cdffc436a211b0bda58f621b";>f0fb958 [meta] create FUNDING.yml; add funding in package.json https://github.com/minimistjs/minimist/commit/3639e0c819359a366387e425ab6eabf4c78d3caa";>3639e0c [meta] use npmignore to autogenerate an npmignore file https://github.com/minimistjs/minimist/commit/be2e038c342d8333b32f0fde67a0026b79c8150e";>be2e038 Only apps should have lockfiles https://github.com/minimistjs/minimist/commit/282b570e7489d01b03f2d6d3dabf79cd3e5f84cf";>282b570 isConstructorOrProto adapted from PR https://github.com/minimistjs/minimist/commit/ef9153fc52b6cea0744b2239921c5dcae4697f11";>ef9153f [Dev Deps] update @ljharb/eslint-config, aud https://github.com/minimistjs/minimist/commit/098873c213cdb7c92e55ae1ef5aa1af3a8192a79";>098873c [Dev Deps] update @ljharb/eslint-config, aud https://github.com/minimistjs/minimist/commit/3124ed3e46306301ebb3c834874ce0241555c2c4";>3124ed3 [meta] add safe-publish-latest https://github.com/minimistjs/minimist/commit/4b927de696d561c636b4f43bf49d4597cb36d6d6";>4b927de [T
[GitHub] [tomcat-training] dependabot[bot] commented on pull request #3: Bump minimist from 0.0.8 to 1.2.6
dependabot[bot] commented on PR #3: URL: https://github.com/apache/tomcat-training/pull/3#issuecomment-1447486061 Superseded by #13. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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-training] dependabot[bot] closed pull request #3: Bump minimist from 0.0.8 to 1.2.6
dependabot[bot] closed pull request #3: Bump minimist from 0.0.8 to 1.2.6 URL: https://github.com/apache/tomcat-training/pull/3 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] pangxianhai opened a new pull request, #594: Update CoyoteAdapter.java
pangxianhai opened a new pull request, #594: URL: https://github.com/apache/tomcat/pull/594 we have a problem at function parseSessionCookiesId. If the request has two JSESSIONID on the Cookie. The session will init twice. So request.isRequestedSessionIdValid() will be invoked. But at request.isRequestedSessionIdValid() , it will call manager.findSession(requestedSessionId). This time request is not bind ClassLoader. We customized the manager. On the manager, will used some class at War. So it's will throw a ClassNotFound exception. So I think the session only need init once. Because the second init is the same as the first time. If request.isRequestedSessionIdValid return true,the second is not need. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] pangxianhai commented on pull request #594: Update CoyoteAdapter.java
pangxianhai commented on PR #594: URL: https://github.com/apache/tomcat/pull/594#issuecomment-1452803563 we have a problem at function parseSessionCookiesId. If the request has two JSESSIONID on the Cookie. The session will init twice. So request.isRequestedSessionIdValid() will be invoked. But at request.isRequestedSessionIdValid() , it will call manager.findSession(requestedSessionId). This time request is not bind ClassLoader. We customized the manager. On the manager, will used some class at War. So it's will throw a ClassNotFound exception. So I think the session only need init once. Because the second init is the same as the first time. If request.isRequestedSessionIdValid return true,the second is not need. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #594: Update CoyoteAdapter.java
markt-asf commented on PR #594: URL: https://github.com/apache/tomcat/pull/594#issuecomment-1453068854 Thanks for the PR. I understand the problem but this isn't the right fix. It will break apps when there are two session cookies and the second one is the only valid one. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #539: JsonAccessLogValve: Add valve for structured logging
rmaucher commented on PR #539: URL: https://github.com/apache/tomcat/pull/539#issuecomment-1453257058 I merged this with significant modifications. Thanks. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 #539: JsonAccessLogValve: Add valve for structured logging
rmaucher closed pull request #539: JsonAccessLogValve: Add valve for structured logging URL: https://github.com/apache/tomcat/pull/539 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] happyhua commented on pull request #581: Speedup by removing non pattern replaceAll with constant arg
happyhua commented on PR #581:
URL: https://github.com/apache/tomcat/pull/581#issuecomment-1454685803
A replaceAll -> replace change in for example
`java/org/apache/tomcat/buildutil/translate/Utils.java` can produce different
output.
```
String source = "1\t2\t3";
System.out.println("source: " + source);
System.out.println("replaceAll: " + source.replaceAll("\t", "\\t"));
System.out.println("replace: " + source.replace("\t", "\\t"));
```
```
source: 12 3
replaceAll: 1t2t3
replace: 1\t2\t3
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] aooohan commented on pull request #595: Exception message error
aooohan commented on PR #595: URL: https://github.com/apache/tomcat/pull/595#issuecomment-1457597852 Thanks for reporting this. However, I found differences between this file and spec, so I will update it to align with spec. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan closed pull request #595: Exception message error
aooohan closed pull request #595: Exception message error URL: https://github.com/apache/tomcat/pull/595 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] jbisotti opened a new issue, #44: Does this work tool work at the source or binary level?
jbisotti opened a new issue, #44: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/44 From the documentation, it is unclear to me if this tools updates source code or if it manipulates bytecode. Which is it? Thanks. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org.apache.org 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] ebourg commented on issue #44: Does this work tool work at the source or binary level?
ebourg commented on issue #44: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/44#issuecomment-1459025587 It works at both source and binary level, recursively. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 closed issue #44: Does this work tool work at the source or binary level?
markt-asf closed issue #44: Does this work tool work at the source or binary level? URL: https://github.com/apache/tomcat-jakartaee-migration/issues/44 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 opened a new pull request, #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
ChristopherSchultz opened a new pull request, #596: URL: https://github.com/apache/tomcat/pull/596 I have only compile-tested this; I wanted to get feedback on the approach, how to handle errors, etc. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 opened a new pull request, #597: Use a deep copy of query stats whose values won't change during sorting.
ChristopherSchultz opened a new pull request, #597: URL: https://github.com/apache/tomcat/pull/597 Fixes https://bz.apache.org/bugzilla/show_bug.cgi?id=58489 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] isapir commented on pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
isapir commented on PR #596: URL: https://github.com/apache/tomcat/pull/596#issuecomment-1460841013 @ChristopherSchultz Is there a list of supported database systems with which the DataSourceStore is compatible? Are you sure that they all support "SELECT FOR UPDATE"? I tried to look that up and the "best" thing I found was https://www.sql-workbench.eu/dbms_comparison.html -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] isapir commented on a diff in pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
isapir commented on code in PR #596:
URL: https://github.com/apache/tomcat/pull/596#discussion_r1130013002
##
java/org/apache/catalina/session/DataSourceStore.java:
##
@@ -626,15 +626,77 @@ public void save(Session session) throws IOException {
byte[] obs = bos.toByteArray();
int size = obs.length;
try (ByteArrayInputStream bis = new
ByteArrayInputStream(obs, 0, size);
-InputStream in = new BufferedInputStream(bis,
size);
-PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql)) {
-preparedSaveSql.setString(1, session.getIdInternal());
-preparedSaveSql.setString(2, getName());
-preparedSaveSql.setBinaryStream(3, in, size);
-preparedSaveSql.setString(4, session.isValid() ? "1" :
"0");
-preparedSaveSql.setInt(5,
session.getMaxInactiveInterval());
-preparedSaveSql.setLong(6,
session.getLastAccessedTime());
-preparedSaveSql.execute();
+ InputStream in = new BufferedInputStream(bis, size);
+ PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql, ResultSet.TYPE_FORWARD_ONLY,
ResultSet.CONCUR_UPDATABLE)) {
+
+// Store auto-commit state
+boolean autoCommit = _conn.getAutoCommit();
+
+try {
+if(autoCommit) {
+_conn.setAutoCommit(false); // BEGIN
TRANSACTION
+}
+
+preparedSaveSql.setString(1, getName());
+preparedSaveSql.setString(2,
session.getIdInternal());
+
+ResultSet rs = preparedSaveSql.executeQuery();
+
+if(rs.next()) {
+// Session already exists in the db; update
the various fields
+rs.updateBinaryStream(sessionDataCol, in,
size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+} else {
+// Session does not exist. Insert.
+rs.moveToInsertRow();
+
+rs.updateString(sessionAppCol, getName());
+rs.updateString(sessionIdCol,
session.getIdInternal());
+rs.updateBinaryStream(sessionIdCol, in, size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+}
+
+_conn.commit();
+} catch (SQLException sqle) {
Review Comment:
Is it possible to put all the handlers in the same block [1]? e.g.
```
} catch (SQLException | RuntimeException | Error e) {
// looks like mostly the same block to me
}
```
We don't need to support Java 6
[1]
https://docs.oracle.com/javase/7/docs/technotes/guides/language/catch-multiple.html
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] aooohan commented on a diff in pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
aooohan commented on code in PR #596:
URL: https://github.com/apache/tomcat/pull/596#discussion_r1130309107
##
java/org/apache/catalina/session/DataSourceStore.java:
##
@@ -626,15 +626,77 @@ public void save(Session session) throws IOException {
byte[] obs = bos.toByteArray();
int size = obs.length;
try (ByteArrayInputStream bis = new
ByteArrayInputStream(obs, 0, size);
-InputStream in = new BufferedInputStream(bis,
size);
-PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql)) {
-preparedSaveSql.setString(1, session.getIdInternal());
-preparedSaveSql.setString(2, getName());
-preparedSaveSql.setBinaryStream(3, in, size);
-preparedSaveSql.setString(4, session.isValid() ? "1" :
"0");
-preparedSaveSql.setInt(5,
session.getMaxInactiveInterval());
-preparedSaveSql.setLong(6,
session.getLastAccessedTime());
-preparedSaveSql.execute();
+ InputStream in = new BufferedInputStream(bis, size);
+ PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql, ResultSet.TYPE_FORWARD_ONLY,
ResultSet.CONCUR_UPDATABLE)) {
+
+// Store auto-commit state
+boolean autoCommit = _conn.getAutoCommit();
+
+try {
+if(autoCommit) {
+_conn.setAutoCommit(false); // BEGIN
TRANSACTION
+}
+
+preparedSaveSql.setString(1, getName());
+preparedSaveSql.setString(2,
session.getIdInternal());
+
+ResultSet rs = preparedSaveSql.executeQuery();
+
+if(rs.next()) {
+// Session already exists in the db; update
the various fields
+rs.updateBinaryStream(sessionDataCol, in,
size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+} else {
+// Session does not exist. Insert.
+rs.moveToInsertRow();
+
+rs.updateString(sessionAppCol, getName());
+rs.updateString(sessionIdCol,
session.getIdInternal());
+rs.updateBinaryStream(sessionIdCol, in, size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
Review Comment:
```suggestion
rs.insertRow();
```
Here is not correct.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
ChristopherSchultz commented on PR #596: URL: https://github.com/apache/tomcat/pull/596#issuecomment-1462052065 > @ChristopherSchultz Is there a list of supported database systems with which the DataSourceStore is compatible? Are you sure that they all support "SELECT FOR UPDATE"? I tried to look that up and the "best" thing I found was https://www.sql-workbench.eu/dbms_comparison.html Good question. I was originally going to use `MERGE` but the database I have at-hand (MariaDB) doesn't support it, so I went with `SELECT...FOR UPDATE`. It's not clear to me if the `SELECT...FOR UPDATE NOWAIT` listed in that table is specifically talking about the `NOWAIT` part of the query. For example, I know that both MariaDB and MySQL have supported `SELECT...FOR UPDATE` for a long time, but the entry for MySQL states "_Since 8.0_" and MariaDB says "_No_". A quick check of the [HSQLDB Documentation](http://hsqldb.org/doc/2.0/guide/dataaccess-chapt.html#dac_jdbc_cursor_updatability) shows that `SELECT...FOR UPDATE` is available while that list claims it is not supported. Same thing with [IBM DB2](https://www.ibm.com/docs/en/informix-servers/12.10?topic=statement-update-clause) and [SQL Server](https://learn.microsoft.com/en-us/sql/odbc/reference/appendixes/processing-select-for-update-statements?view=sql-server-ver16). But it's clear that not all RDMBSs support that syntax. Maybe it's not possible to write a single code-path to satisfy all major RDBMS systems. I'd be happy to change the PR to offer a selection between the two via configuration. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 diff in pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
ChristopherSchultz commented on code in PR #596:
URL: https://github.com/apache/tomcat/pull/596#discussion_r1131019399
##
java/org/apache/catalina/session/DataSourceStore.java:
##
@@ -626,15 +626,77 @@ public void save(Session session) throws IOException {
byte[] obs = bos.toByteArray();
int size = obs.length;
try (ByteArrayInputStream bis = new
ByteArrayInputStream(obs, 0, size);
-InputStream in = new BufferedInputStream(bis,
size);
-PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql)) {
-preparedSaveSql.setString(1, session.getIdInternal());
-preparedSaveSql.setString(2, getName());
-preparedSaveSql.setBinaryStream(3, in, size);
-preparedSaveSql.setString(4, session.isValid() ? "1" :
"0");
-preparedSaveSql.setInt(5,
session.getMaxInactiveInterval());
-preparedSaveSql.setLong(6,
session.getLastAccessedTime());
-preparedSaveSql.execute();
+ InputStream in = new BufferedInputStream(bis, size);
+ PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql, ResultSet.TYPE_FORWARD_ONLY,
ResultSet.CONCUR_UPDATABLE)) {
+
+// Store auto-commit state
+boolean autoCommit = _conn.getAutoCommit();
+
+try {
+if(autoCommit) {
+_conn.setAutoCommit(false); // BEGIN
TRANSACTION
+}
+
+preparedSaveSql.setString(1, getName());
+preparedSaveSql.setString(2,
session.getIdInternal());
+
+ResultSet rs = preparedSaveSql.executeQuery();
+
+if(rs.next()) {
+// Session already exists in the db; update
the various fields
+rs.updateBinaryStream(sessionDataCol, in,
size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+} else {
+// Session does not exist. Insert.
+rs.moveToInsertRow();
+
+rs.updateString(sessionAppCol, getName());
+rs.updateString(sessionIdCol,
session.getIdInternal());
+rs.updateBinaryStream(sessionIdCol, in, size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
Review Comment:
Duh, thanks!
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 diff in pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
ChristopherSchultz commented on code in PR #596:
URL: https://github.com/apache/tomcat/pull/596#discussion_r1131033698
##
java/org/apache/catalina/session/DataSourceStore.java:
##
@@ -626,15 +626,77 @@ public void save(Session session) throws IOException {
byte[] obs = bos.toByteArray();
int size = obs.length;
try (ByteArrayInputStream bis = new
ByteArrayInputStream(obs, 0, size);
-InputStream in = new BufferedInputStream(bis,
size);
-PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql)) {
-preparedSaveSql.setString(1, session.getIdInternal());
-preparedSaveSql.setString(2, getName());
-preparedSaveSql.setBinaryStream(3, in, size);
-preparedSaveSql.setString(4, session.isValid() ? "1" :
"0");
-preparedSaveSql.setInt(5,
session.getMaxInactiveInterval());
-preparedSaveSql.setLong(6,
session.getLastAccessedTime());
-preparedSaveSql.execute();
+ InputStream in = new BufferedInputStream(bis, size);
+ PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql, ResultSet.TYPE_FORWARD_ONLY,
ResultSet.CONCUR_UPDATABLE)) {
+
+// Store auto-commit state
+boolean autoCommit = _conn.getAutoCommit();
+
+try {
+if(autoCommit) {
+_conn.setAutoCommit(false); // BEGIN
TRANSACTION
+}
+
+preparedSaveSql.setString(1, getName());
+preparedSaveSql.setString(2,
session.getIdInternal());
+
+ResultSet rs = preparedSaveSql.executeQuery();
+
+if(rs.next()) {
+// Session already exists in the db; update
the various fields
+rs.updateBinaryStream(sessionDataCol, in,
size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+} else {
+// Session does not exist. Insert.
+rs.moveToInsertRow();
+
+rs.updateString(sessionAppCol, getName());
+rs.updateString(sessionIdCol,
session.getIdInternal());
+rs.updateBinaryStream(sessionIdCol, in, size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+}
+
+_conn.commit();
+} catch (SQLException sqle) {
Review Comment:
I would be okay merging them together, but we might want to use different
error messages. This is one of the things I wanted to get feedback on: how
important is it to say "we got an Error" vs "we got an SQLException", etc. when
the actual exception and stack trace will likely be logged? I think it's
probably okay to merge these exception handlers together but wanted some
feedback about the logging.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] isapir commented on a diff in pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
isapir commented on code in PR #596:
URL: https://github.com/apache/tomcat/pull/596#discussion_r1131364805
##
java/org/apache/catalina/session/DataSourceStore.java:
##
@@ -626,15 +626,77 @@ public void save(Session session) throws IOException {
byte[] obs = bos.toByteArray();
int size = obs.length;
try (ByteArrayInputStream bis = new
ByteArrayInputStream(obs, 0, size);
-InputStream in = new BufferedInputStream(bis,
size);
-PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql)) {
-preparedSaveSql.setString(1, session.getIdInternal());
-preparedSaveSql.setString(2, getName());
-preparedSaveSql.setBinaryStream(3, in, size);
-preparedSaveSql.setString(4, session.isValid() ? "1" :
"0");
-preparedSaveSql.setInt(5,
session.getMaxInactiveInterval());
-preparedSaveSql.setLong(6,
session.getLastAccessedTime());
-preparedSaveSql.execute();
+ InputStream in = new BufferedInputStream(bis, size);
+ PreparedStatement preparedSaveSql =
_conn.prepareStatement(saveSql, ResultSet.TYPE_FORWARD_ONLY,
ResultSet.CONCUR_UPDATABLE)) {
+
+// Store auto-commit state
+boolean autoCommit = _conn.getAutoCommit();
+
+try {
+if(autoCommit) {
+_conn.setAutoCommit(false); // BEGIN
TRANSACTION
+}
+
+preparedSaveSql.setString(1, getName());
+preparedSaveSql.setString(2,
session.getIdInternal());
+
+ResultSet rs = preparedSaveSql.executeQuery();
+
+if(rs.next()) {
+// Session already exists in the db; update
the various fields
+rs.updateBinaryStream(sessionDataCol, in,
size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+} else {
+// Session does not exist. Insert.
+rs.moveToInsertRow();
+
+rs.updateString(sessionAppCol, getName());
+rs.updateString(sessionIdCol,
session.getIdInternal());
+rs.updateBinaryStream(sessionIdCol, in, size);
+rs.updateString(sessionValidCol,
session.isValid() ? "1" : "0");
+rs.updateInt(sessionMaxInactiveCol,
session.getMaxInactiveInterval());
+rs.updateLong(sessionLastAccessedCol,
session.getLastAccessedTime());
+
+rs.updateRow();
+}
+
+_conn.commit();
+} catch (SQLException sqle) {
Review Comment:
The Stack Trace would be the same, true, but the exception class and message
would still provide the details
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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] isapir commented on pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
isapir commented on PR #596: URL: https://github.com/apache/tomcat/pull/596#issuecomment-1462467192 Yeah, I guess that site is not up to date. I also used SELECT FOR UPDATE in MySQL 5.7. There is also INSERT ON CONFLICT UPDATE support in MySQL and Postgres, but it would be difficult to find an optimized solution that fits all. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan commented on pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
aooohan commented on PR #596: URL: https://github.com/apache/tomcat/pull/596#issuecomment-1463201626 I have a question that why we don't add a real **primary key**(auto-increment) to solve the problem that primary key constraint violation when insert data to database simultaneously? And we can select session data from table by **session-id** and order (DSC) by **ID** (real primary key) when load session, then the newest result is what we need. Thus, we can avoid adding lock(FOR UPDATE or others) from the database level by this way. I think this will work and so simple, but I don't know if there will be any security issues. Thoughts? -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan closed pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
aooohan closed pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w" URL: https://github.com/apache/tomcat/pull/592 -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] aooohan commented on pull request #592: Improved regexp performance: "a-zA-Z0-9_" -> "\w"
aooohan commented on PR #592: URL: https://github.com/apache/tomcat/pull/592#issuecomment-1463387623 Merge manually, thanks. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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 pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
ChristopherSchultz commented on PR #596: URL: https://github.com/apache/tomcat/pull/596#issuecomment-1463902816 The problem is that there is a window of opportunity between the existing `DELETE` and `INSERT` where the `session_id` column (which is `UNIQUE` or equivalent) can be `INSERT`ed by another thread. Changing the definition of the table to include a separate PK and removing the `UNIQUE` constraint from the `session_id` column is one way to solve this issue, but it would break any existing installation which is _not_ using that kind of table definition. I would definitely *not* want to do that for a point-release, and my goal here is to back-port this all the way back to 8.5 at this point. I think some larger changes could be made separately from this current effort, and restricted to maybe Tomcat 11.0.x and later. -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] FSchumacher commented on a diff in pull request #597: Use a deep copy of query stats whose values won't change during sorting.
FSchumacher commented on code in PR #597:
URL: https://github.com/apache/tomcat/pull/597#discussion_r1133086091
##
modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java:
##
@@ -222,19 +222,55 @@ protected QueryStats getQueryStats(String sql) {
return qs;
}
+static class MiniQueryStats {
+public final QueryStats queryStats;
+public final long lastInvocation;
+
+public MiniQueryStats(QueryStats queryStats) {
+this.queryStats = queryStats;
+this.lastInvocation = queryStats.lastInvocation;
+}
+}
+
+static class MiniQueryStatsComparator implements Comparator
+{
+@Override
+public int compare(MiniQueryStats stats1, MiniQueryStats stats2) {
+return Long.compare(handleZero(stats1.lastInvocation),
+handleZero(stats2.lastInvocation));
+}
+
+private static long handleZero(long value) {
+return value == 0 ? Long.MAX_VALUE : value;
+}
+}
+
+private MiniQueryStatsComparator miniQueryStatsComparator = new
MiniQueryStatsComparator();
+
/**
* Sort QueryStats by last invocation time
* @param queries The queries map
*/
protected void removeOldest(ConcurrentHashMap queries) {
-ArrayList list = new ArrayList<>(queries.values());
-Collections.sort(list, queryStatsComparator);
+// Make a defensive deep-copy of the query stats list to prevent
+// concurrent changes to the lastModified member during list-sort
+ArrayList list = new ArrayList<>(queries.size());
+for(QueryStats stats : queries.values()) {
+list.add(new MiniQueryStats(stats));
+}
+
+Collections.sort(list, miniQueryStatsComparator);
int removeIndex = 0;
while (queries.size() > maxQueries) {
-String sql = list.get(removeIndex).getQuery();
-queries.remove(sql);
-if (log.isDebugEnabled()) {
- log.debug("Removing slow query, capacity reached:"+sql);
+MiniQueryStats mqs = list.get(removeIndex);
+// Check to see if the lastInvocation has been updated since we
+// took our snapshot. If the timestamps disagree, it means
+// that this item is no longer the oldest (and it likely now
+// one of the newest).
+if(mqs.lastInvocation == mqs.queryStats.lastInvocation) {
Review Comment:
What happens, when all (or enough) of the stats have been changed in
between? Wouldn't we get a IndexOutOfBoundException? May be, we should add a
guard to the while expression and log a message, that we could net free enough
stats.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 diff in pull request #597: Use a deep copy of query stats whose values won't change during sorting.
ChristopherSchultz commented on code in PR #597:
URL: https://github.com/apache/tomcat/pull/597#discussion_r1133119845
##
modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java:
##
@@ -222,19 +222,55 @@ protected QueryStats getQueryStats(String sql) {
return qs;
}
+static class MiniQueryStats {
+public final QueryStats queryStats;
+public final long lastInvocation;
+
+public MiniQueryStats(QueryStats queryStats) {
+this.queryStats = queryStats;
+this.lastInvocation = queryStats.lastInvocation;
+}
+}
+
+static class MiniQueryStatsComparator implements Comparator
+{
+@Override
+public int compare(MiniQueryStats stats1, MiniQueryStats stats2) {
+return Long.compare(handleZero(stats1.lastInvocation),
+handleZero(stats2.lastInvocation));
+}
+
+private static long handleZero(long value) {
+return value == 0 ? Long.MAX_VALUE : value;
+}
+}
+
+private MiniQueryStatsComparator miniQueryStatsComparator = new
MiniQueryStatsComparator();
+
/**
* Sort QueryStats by last invocation time
* @param queries The queries map
*/
protected void removeOldest(ConcurrentHashMap queries) {
-ArrayList list = new ArrayList<>(queries.values());
-Collections.sort(list, queryStatsComparator);
+// Make a defensive deep-copy of the query stats list to prevent
+// concurrent changes to the lastModified member during list-sort
+ArrayList list = new ArrayList<>(queries.size());
+for(QueryStats stats : queries.values()) {
+list.add(new MiniQueryStats(stats));
+}
+
+Collections.sort(list, miniQueryStatsComparator);
int removeIndex = 0;
while (queries.size() > maxQueries) {
-String sql = list.get(removeIndex).getQuery();
-queries.remove(sql);
-if (log.isDebugEnabled()) {
- log.debug("Removing slow query, capacity reached:"+sql);
+MiniQueryStats mqs = list.get(removeIndex);
+// Check to see if the lastInvocation has been updated since we
+// took our snapshot. If the timestamps disagree, it means
+// that this item is no longer the oldest (and it likely now
+// one of the newest).
+if(mqs.lastInvocation == mqs.queryStats.lastInvocation) {
Review Comment:
Good question. If 100% of the stats have been updated, then we will remove
nothing, no space will be gained, and we will end up running off the end of the
list, throwing an error.
I see a few options:
1. YOLO! Just let the loop run and hope for the best! j/k
2. Ensure `removeIndex` doesn't grow too high, then just stop
3. Do (2), and repeat the sort+loop until we remove enough entries to meet
cache-size expectations
I think probably (2) is sufficient. Thoughts?
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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 pull request #596: Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT.
ChristopherSchultz commented on PR #596: URL: https://github.com/apache/tomcat/pull/596#issuecomment-1464952161 I've been thinking about this more and I think it could cause problems for people not using appId+sessionId (or just sessionId) as the primary key for the DB table storing the sessions. `INSERT` and `DELETE` are not required to include a value for the PK. Some databases can automatically-allocate a value for that purpose and so the existing `DELETE`+`INSERT` scheme works for them. But the `SELECT ... FOR UPDATE` will not because it does not include the table's PK. One way to fix this would be to add an optional PK column (columns?) to the configuration. I think this is pretty important, because databases which use PK-indexed organization (SQL Server, InnoDB, and others) will thrash-around if appId+sessionId are the PK columns, so anybody using those kinds of DBs would likely choose a separate PK column that doesn't cause those ugly performance problems. Any other thoughts? -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org 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] FSchumacher commented on a diff in pull request #597: Use a deep copy of query stats whose values won't change during sorting.
FSchumacher commented on code in PR #597:
URL: https://github.com/apache/tomcat/pull/597#discussion_r1133134054
##
modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java:
##
@@ -222,19 +222,55 @@ protected QueryStats getQueryStats(String sql) {
return qs;
}
+static class MiniQueryStats {
+public final QueryStats queryStats;
+public final long lastInvocation;
+
+public MiniQueryStats(QueryStats queryStats) {
+this.queryStats = queryStats;
+this.lastInvocation = queryStats.lastInvocation;
+}
+}
+
+static class MiniQueryStatsComparator implements Comparator
+{
+@Override
+public int compare(MiniQueryStats stats1, MiniQueryStats stats2) {
+return Long.compare(handleZero(stats1.lastInvocation),
+handleZero(stats2.lastInvocation));
+}
+
+private static long handleZero(long value) {
+return value == 0 ? Long.MAX_VALUE : value;
+}
+}
+
+private MiniQueryStatsComparator miniQueryStatsComparator = new
MiniQueryStatsComparator();
+
/**
* Sort QueryStats by last invocation time
* @param queries The queries map
*/
protected void removeOldest(ConcurrentHashMap queries) {
-ArrayList list = new ArrayList<>(queries.values());
-Collections.sort(list, queryStatsComparator);
+// Make a defensive deep-copy of the query stats list to prevent
+// concurrent changes to the lastModified member during list-sort
+ArrayList list = new ArrayList<>(queries.size());
+for(QueryStats stats : queries.values()) {
+list.add(new MiniQueryStats(stats));
+}
+
+Collections.sort(list, miniQueryStatsComparator);
int removeIndex = 0;
while (queries.size() > maxQueries) {
-String sql = list.get(removeIndex).getQuery();
-queries.remove(sql);
-if (log.isDebugEnabled()) {
- log.debug("Removing slow query, capacity reached:"+sql);
+MiniQueryStats mqs = list.get(removeIndex);
+// Check to see if the lastInvocation has been updated since we
+// took our snapshot. If the timestamps disagree, it means
+// that this item is no longer the oldest (and it likely now
+// one of the newest).
+if(mqs.lastInvocation == mqs.queryStats.lastInvocation) {
Review Comment:
I had number two in mind, with a warn message at the end, when we could not
free enough stats.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
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
