[GitHub] [tomcat] stokito opened a new pull request #325: Simplify ETag check

classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito opened a new pull request #325: Simplify ETag check

GitBox

stokito opened a new pull request #325:
URL: https://github.com/apache/tomcat/pull/325


   The If-None-Match header many have multiple ETags https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26
   Even if Tomcat returns a single ETag clients may want to send few of them.
   Since ETags are always quoted the easiest way to check is just to check a substring.
   
   This will reduce amount of memory consumed by full ETag parsing.
   
   A command to check:
   
       curl -vv http://localhost:8080/tomcat.png -H 'If-None-Match: W/"5103-1595246577000", "etag2"'
   
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check

GitBox

reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661095956


   The suggested change will cause incorrect behaviour, for instance
   
       "foobar"
   
   would match
   
       W/"foobar"
   
   That said, the existing code is broken as well, as it will fail if an ETag contains a comma.


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661111650


   Thank you, good points! I checked spec https://tools.ietf.org/html/rfc7232#section-2.3.2 and it looks like we still fine here because Tomcat in fact should do a weak comparison.
   
   |If-None-Match | Real resource's ETag | Match              |
   |--------------------|-----------------------------|---------------------|
   |`W/"1"`            | `W/"1"`                       | Yes                   |
   |`"1"`                | `W/"1"`                       | No.                    |
   |`W/"1"`            | `"1"`                           | Yes, as in spec |
   |`"1"`                | `"1"`                           | Yes                    |
   |`W/"1", "1"`     | `"1"`                           | Yes                    |
   |`W/"1", "2"`     | `W/"2"`                       | No                     |
   
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661111650


   Thank you, good points! I checked spec https://tools.ietf.org/html/rfc7232#section-2.3.2 and it looks like we still fine here because Tomcat in fact should do a weak comparison.
   
   |If-None-Match | Real resource's ETag | Match              |
   |--------------------|-----------------------------|---------------------|
   |`W/"1"`            | `W/"1"`                       | Yes                   |
   |`"1"`                | `W/"1"`                       | No.                    |
   |`W/"1"`            | `"1"`                           | Yes, as in spec |
   |`"1"`                | `"1"`                           | Yes                    |
   |`W/"1", "1"`     | `"1"`                           | Yes                    |
   |`W/"1", "2"`     | `W/"2"`                       | No                     |
   
   A Comma inside of ETag won’t be a problem.
   In fact this is simple, fast and straightforward algorithm.


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661164935


   You are citing the spec very selectively (hint: there are strong and weak comparion functions).


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] reschke edited a comment on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

reschke edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661164935


   You are citing the spec very selectively (hint: there are strong and weak comparison functions).


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661232860


   Yeah, that's why I said "in fact should do". This is not a big deal while Tomcat generates those ETag itself. I'm not even sure that there is any clients who sends multiple ETags. But the change improves performance a lot.


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661279813


   > But the change improves performance a lot.
   
   Can you quantify that?


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600


   At least twice fast because and creates less memory garbage.
   Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
   I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600


   At least twice fast because creates less memory garbage.
   Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
   I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600


   At least twice fast because creates less memory garbage. For two comma separated ETags may work 5 times faster.
   Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
   I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661356032


   I found related ticket https://bz.apache.org/bugzilla/show_bug.cgi?id=64265


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661607221


   > At least twice fast because creates less memory garbage
   
   And this is measurable in practice., not in an isolated unit test?
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661356032


   a related ticket https://bz.apache.org/bugzilla/show_bug.cgi?id=64265


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #325: BZ 64265 Fix and simplify weak ETag matching

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600


   At least triple fast because creates less memory garbage. For two comma separated ETags may work 10 times faster.
   Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
   I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching

GitBox
In reply to this post by GitBox

stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666382555


   Ok, so Tomcat should make a weak matching.
   I changed the description of PR and added commits. Now this is a bug fix and simplification is only in last commit.
   You can merge everything except of the last commit d4831ba.
   
   Since logic of matching now even more complicated I updated the benchmark:
   https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
   But more important is that it just looks simpler.


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching

GitBox
In reply to this post by GitBox

stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666445794


   I added another two commits with performance optimization. Given the nature of ETags that Tomcat generates itself I made some priority to check ETag:
   1. Full match (what we normally expect)
   2. Weak match
   3. Asterisk match (not sure who ever used it)


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #325: BZ 64265 Fix and simplify weak ETag matching

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666445794


   I added another two commits with performance optimization. Given the nature of ETags that Tomcat generates itself I made some priority to check ETag:
   1. Strict comparision (what we normally expect)
   2. Weak comparision
   3. Asterisk (not sure who ever used it)


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching

GitBox
In reply to this post by GitBox

stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666645463


   OMG you right, they differ:
   
   
        An origin server MUST use the strong comparison function when
        comparing entity-tags for If-Match (Section 2.3.2), since the client
        intends this precondition to prevent the method from being applied if
        there have been any changes to the representation data.
   
        A recipient MUST use the weak comparison function when comparing
        entity-tags for If-None-Match (Section 2.3.2), since weak entity-tags
        can be used for cache validation even if there have been changes to
        the representation data.
   
   But what is funny is that in Tomcat on the contrary `If-Match` is *weak* while  `If-None-Match` is *strong*.
   
   I'll fix that and update the PR


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] stokito edited a comment on pull request #325: BZ 64265 Fix and simplify weak ETag matching

GitBox
In reply to this post by GitBox

stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666645463


   OMG you right, they differ:
   
   
        An origin server MUST use the strong comparison function when
        comparing entity-tags for If-Match (Section 2.3.2), since the client
        intends this precondition to prevent the method from being applied if
        there have been any changes to the representation data.
   
   
        A recipient MUST use the weak comparison function when comparing
        entity-tags for If-None-Match (Section 2.3.2), since weak entity-tags
        can be used for cache validation even if there have been changes to
        the representation data.
   
   But what is funny is that in Tomcat on the contrary `If-Match` is *weak* while  `If-None-Match` is *strong*.
   
   I'll fix that and update the PR


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12