You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2023/03/13 22:07:09 UTC
[GitHub] [httpcomponents-client] arturobernalg opened a new pull request, #424: Fix issue with duplicate parsing of Cache-Control header
arturobernalg opened a new pull request, #424:
URL: https://github.com/apache/httpcomponents-client/pull/424
This PR fixes a bug where the same Cache-Control [f3f07a3](https://github.com/apache/httpcomponents-client/pull/420) header is parsed twice, leading to incorrect performance issues. The parsing code has been extracted from the calculateFreshnessLifetime method and is now enhanced to include the main cache control directive that the isExplicitlyNonCacheable method uses to make its decision. This makes the decision based on the main cache control directive instead of parsing the header twice.
Additionally, this PR adds a new method parseCacheControlHeader to the CacheControlParser class which parses the Cache-Control header and returns a CacheControl instance. This new method improves the parsing of the Cache-Control header and provides more accurate parsing of the different directives.
This PR also includes some code cleanup and refactoring.
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on a diff in pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#discussion_r1136260505
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -139,12 +138,20 @@ public final CacheControl parse(final Header header) {
while (!cursor.atEnd()) {
final String name = tokenParser.parseToken(buffer, cursor, TOKEN_DELIMS);
+
Review Comment:
@ok2c
I agree, however, I am open to any help or advice as it has been challenging to fit all the pieces together. Nevertheless, I am determined to find the optimal solution.
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on a diff in pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#discussion_r1136572957
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -139,12 +138,20 @@ public final CacheControl parse(final Header header) {
while (!cursor.atEnd()) {
final String name = tokenParser.parseToken(buffer, cursor, TOKEN_DELIMS);
+
Review Comment:
> @arturobernalg Oh, man. This looks ugly. There should be a better way of doing the same.
@ok2c what about now?
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#issuecomment-1470651712
> 8ba0b17
Hi @ok2c
The change look good.
now I'm lost here -->"And why on earth did you choose to use deprecated DateUtils methods in your tests?"
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on a diff in pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#discussion_r1136260505
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -139,12 +138,20 @@ public final CacheControl parse(final Header header) {
while (!cursor.atEnd()) {
final String name = tokenParser.parseToken(buffer, cursor, TOKEN_DELIMS);
+
Review Comment:
@ok2c
Perhaps, however, I am open to any help or advice as it has been challenging to fit all the pieces together. Nevertheless, I am determined to find the optimal solution.
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c closed pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c closed pull request #424: Fix issue with duplicate parsing of Cache-Control header
URL: https://github.com/apache/httpcomponents-client/pull/424
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#issuecomment-1470665201
> @arturobernalg
>
> ```
> [INFO] --- maven-compiler-plugin:3.10.1:testCompile (default-testCompile) @ httpclient5-cache ---
> [INFO] Toolchain in maven-compiler-plugin: JDK[/opt/java-1.8]
> [INFO] Changes detected - recompiling the module!
> [INFO] Compiling 44 source files to /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/target/test-classes
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[920,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[921,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[942,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[943,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[964,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[965,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[986,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> [WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[987,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
> ```
damn.
you right. I'll tackle that later.
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#discussion_r1135790932
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -139,12 +138,20 @@ public final CacheControl parse(final Header header) {
while (!cursor.atEnd()) {
final String name = tokenParser.parseToken(buffer, cursor, TOKEN_DELIMS);
+
Review Comment:
@arturobernalg Oh, man. This looks ugly. There should be a better way of doing the same.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -85,7 +83,7 @@ class CacheControlHeaderParser {
/**
* The set of characters that can delimit a value in the header.
*/
- private static final BitSet VALUE_DELIMS = Tokenizer.INIT_BITSET(EQUAL_CHAR, ',');
+ private static final BitSet VALUE_DELIMS = Tokenizer.INIT_BITSET(EQUAL_CHAR, ',', SEMICOLON_CHAR);
Review Comment:
@arturobernalg Why? Why did you put it back? Where is it being 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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#issuecomment-1470546470
Committed as 8ba0b17aebdcc59a43a3785d751e63cddc2c8a58 with some changes to the parsing code.
@arturobernalg The trick was to reverse the parsing flow and make it simple and linear. Fewer lines of code, fewer intermediate garbage as a result.
I committed by code under your name as going back and forth with each and every line of change would have taken too long. Please take a look at my changes.
And why on earth did you choose to use deprecated `DateUtils` methods in your tests?
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #424: Fix issue with duplicate parsing of Cache-Control header
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #424:
URL: https://github.com/apache/httpcomponents-client/pull/424#issuecomment-1470656592
@arturobernalg
```
[INFO] --- maven-compiler-plugin:3.10.1:testCompile (default-testCompile) @ httpclient5-cache ---
[INFO] Toolchain in maven-compiler-plugin: JDK[/opt/java-1.8]
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 44 source files to /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/target/test-classes
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[920,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[921,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[942,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[943,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[964,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[965,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[986,54] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
[WARNING] /home/oleg/src/apache.org/httpcomponents/httpclient/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java:[987,57] [deprecation] formatDate(Date,String) in DateUtils has been deprecated
```
--
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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org