Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Labels:
      None
    1. deft-52_alt.patch
      39 kB
      Johnathan Meehan
    2. deft-52bis-v2.patch
      17 kB
      Johnathan Meehan

      Activity

      Anonymous created issue -
      Hide
      Ulrich Stärk added a comment -
      Show
      Ulrich Stärk added a comment - Began implementation - https://github.com/johnathanmeehan/deft
      Hide
      Ulrich Stärk added a comment -

      Hi,

      this is my working [implementation](http://github.com/ilmich/deft/commit/3e1e2796de5d906db3d86c7ee01b8be4e107a441).
      If you think that my approach may be acceptable for inclusion in the mainstream, I will continue to work to make it fully standard compliant.

      Regards

      Show
      Ulrich Stärk added a comment - Hi, this is my working [implementation] ( http://github.com/ilmich/deft/commit/3e1e2796de5d906db3d86c7ee01b8be4e107a441 ). If you think that my approach may be acceptable for inclusion in the mainstream, I will continue to work to make it fully standard compliant. Regards
      Hide
      Ulrich Stärk added a comment -

      review in queue...

      Show
      Ulrich Stärk added a comment - review in queue...
      Roger Schildmeijer made changes -
      Field Original Value New Value
      Issue Type Bug [ 1 ] New Feature [ 2 ]
      Priority Major [ 3 ]
      Johnathan Meehan made changes -
      Assignee Johnathan Meehan [ jmeehan ]
      Johnathan Meehan made changes -
      Status Open [ 1 ] In Progress [ 3 ]
      Hide
      Johnathan Meehan added a comment -

      From this it would appear that we have two cookie implementations; my own, and then Michele Zuccalà's (GitHub id "ilmich"). Will spend some time reviewing both and seeing what opportunity there is to take from either to produce a clean patch. I see that Michele's implementation was reviewed however, so perhaps we might just go with that - would we require clearance from Michele for code contributed before Apache if we did that?

      Show
      Johnathan Meehan added a comment - From this it would appear that we have two cookie implementations; my own, and then Michele Zuccalà's (GitHub id "ilmich"). Will spend some time reviewing both and seeing what opportunity there is to take from either to produce a clean patch. I see that Michele's implementation was reviewed however, so perhaps we might just go with that - would we require clearance from Michele for code contributed before Apache if we did that?
      Hide
      Roger Schildmeijer added a comment -

      Yes we need clearance from Michele before we commit his code to the trunk. In this case it should be enough if ilmich attaches the patch and check the "Grant license to ASF for inclusion in ASF works (as per the Apache License §5)" checkbox.

      I recommend to "spend some time reviewing both and seeing what opportunity there is to take from either to produce a clean patch"

      Show
      Roger Schildmeijer added a comment - Yes we need clearance from Michele before we commit his code to the trunk. In this case it should be enough if ilmich attaches the patch and check the "Grant license to ASF for inclusion in ASF works (as per the Apache License §5)" checkbox. I recommend to "spend some time reviewing both and seeing what opportunity there is to take from either to produce a clean patch"
      Hide
      Johnathan Meehan added a comment -

      I have contacted Michele directly, and asked him to upload the patch and comment here. Will start work when this is done.

      Show
      Johnathan Meehan added a comment - I have contacted Michele directly, and asked him to upload the patch and comment here. Will start work when this is done.
      Hide
      Michele Zuccalà added a comment -

      I have attached my patch for this issue.
      As I wrote to Johnathan, little by little
      I modified the original patch with an optimized one (from my point of view)
      Compared to the original, the changes are:

      • no longer uses objects to store cookies (not needed because the cookies are simple strings)
      • methods for setting cookies with more parameters(like Tornado's api) that directly create the "Set-Cookie" headers
      • lazy parsing of request's cookies.

      Thanks!

      Show
      Michele Zuccalà added a comment - I have attached my patch for this issue. As I wrote to Johnathan, little by little I modified the original patch with an optimized one (from my point of view) Compared to the original, the changes are: no longer uses objects to store cookies (not needed because the cookies are simple strings) methods for setting cookies with more parameters(like Tornado's api) that directly create the "Set-Cookie" headers lazy parsing of request's cookies. Thanks!
      Michele Zuccalà made changes -
      Attachment deft-52.patch [ 12487812 ]
      Hide
      Roger Schildmeijer added a comment -

      thanks for your help Michele...very appreciated

      Show
      Roger Schildmeijer added a comment - thanks for your help Michele...very appreciated
      Hide
      Johnathan Meehan added a comment -

      I have attached a patch of my own previous implementation minus unit tests (deft-52_alt.patch), which has been kicked into place and is for discussion only. Not at all near complete.

      Ran through Michele's patch, and it works like a charm. A couple of comments and questions:

      I really liked Michele's idea of storing the cookies as simply strings, and ditching any skeleton object. This needs us to provide a number of setter methods, though - there are currently seven, and that's without other properties liked "discard" and "max-age". The other implementation will return a Cookie object after a set of name/value pair; this is stored by HttpResponse and can be modified through accessors. All cookies are written during HttpResponse#flush. I created a simple example that set, expired and retrieved with both implementations and took the last two runs:

      Object
      Requests per second: 14368.70 /sec (mean)
      Requests per second: 14446.50 /sec (mean)

      String
      Requests per second: 14989.25 /sec (mean)
      Requests per second: 14687.58 /sec (mean)

      Michele's work is faster and straightforward, but the object allows for sanity checking. Does anybody have an opinion on which approach they would prefer? Michele, would you like to try and see what kind of difference you get?

      Specific to Michele's work, I have some notes:

      • I missed the "clearCookie(String)" method, as I was looking for "expireCookie(String)". Just me, or a better name?
      • Would we change the void setter methods to String, to return what we created?
      • When we decide what to do, there're unit tests and Javadoc to be written.
      Show
      Johnathan Meehan added a comment - I have attached a patch of my own previous implementation minus unit tests (deft-52_alt.patch), which has been kicked into place and is for discussion only. Not at all near complete. Ran through Michele's patch, and it works like a charm. A couple of comments and questions: I really liked Michele's idea of storing the cookies as simply strings, and ditching any skeleton object. This needs us to provide a number of setter methods, though - there are currently seven, and that's without other properties liked "discard" and "max-age". The other implementation will return a Cookie object after a set of name/value pair; this is stored by HttpResponse and can be modified through accessors. All cookies are written during HttpResponse#flush. I created a simple example that set, expired and retrieved with both implementations and took the last two runs: Object Requests per second: 14368.70 /sec (mean) Requests per second: 14446.50 /sec (mean) String Requests per second: 14989.25 /sec (mean) Requests per second: 14687.58 /sec (mean) Michele's work is faster and straightforward, but the object allows for sanity checking. Does anybody have an opinion on which approach they would prefer? Michele, would you like to try and see what kind of difference you get? Specific to Michele's work, I have some notes: I missed the "clearCookie(String)" method, as I was looking for "expireCookie(String)". Just me, or a better name? Would we change the void setter methods to String, to return what we created? When we decide what to do, there're unit tests and Javadoc to be written.
      Johnathan Meehan made changes -
      Attachment deft-52_alt.patch [ 12487894 ]
      Hide
      Michele Zuccalà added a comment -

      Johnathan,

      first of all, thanks for your appreciation of my work, I'm happy

      I tried to keep things as simple as possible because my 'old' implementation increased memory consumption, and I love Delft, due to its philosophy of speed with few resources consumed.
      I also tried to follow the new rfc6265, which makes obsolete the property "discard"
      Regarding the "max-age", I know that for now, some browsers ignore it.
      But effectively we can provide a settter with seconds, and still use the property "expires", or something like that

      Regarding the validation may also be done inside the method setCookie ().
      This is because the advantage of having an object for cookies(in my opinion), would be the possibility to check every single set, and generate an exception when an error occurs. But this is perhaps a little uncomfortable for the developer.

      About your notes:

      • I got this name from Tornado's api .
      • do you think could be useful to return the string, with the http header generated, in the setters ?!?!!?
      • right

      Last thing, I noticed a significant increase of performance by applying my patch deft-168 (custom etags, and ability to disable automatic etag generation).
      What do you use for your benchmarks?!
      I use a simple (on linux)

      ab -c 5-t 5 http://localhost:8080/

      launched almost 3 times.

      thanks

      Show
      Michele Zuccalà added a comment - Johnathan, first of all, thanks for your appreciation of my work, I'm happy I tried to keep things as simple as possible because my 'old' implementation increased memory consumption, and I love Delft, due to its philosophy of speed with few resources consumed. I also tried to follow the new rfc6265, which makes obsolete the property "discard" Regarding the "max-age", I know that for now, some browsers ignore it. But effectively we can provide a settter with seconds, and still use the property "expires", or something like that Regarding the validation may also be done inside the method setCookie (). This is because the advantage of having an object for cookies(in my opinion), would be the possibility to check every single set, and generate an exception when an error occurs. But this is perhaps a little uncomfortable for the developer. About your notes: I got this name from Tornado's api . do you think could be useful to return the string, with the http header generated, in the setters ?!?!!? right Last thing, I noticed a significant increase of performance by applying my patch deft-168 (custom etags, and ability to disable automatic etag generation). What do you use for your benchmarks?! I use a simple (on linux) ab -c 5-t 5 http://localhost:8080/ launched almost 3 times. thanks
      Hide
      Johnathan Meehan added a comment - - edited

      > Regarding the "max-age", I know that for now, some browsers ignore it.
      > But effectively we can provide a settter with seconds, and still use the property "expires", or something like that

      Good point, and that's a nice idea. Let's roll with that.

      > I also tried to follow the new rfc6265, which makes obsolete the property "discard"

      EDIT: Removed reference to wrong spec. Sorry, Michele.
      Let's run with your work, Michele - it's fast and ready-to-go. Could you put a final version together for review, with tests, documentation, the fake max-age method and anything else you like? Ping me outside of JIRA if you are short on time and I'll be glad to help out.

      > What do you use for your benchmarks?!
      ab -k -c10 -n250000 http://127.0.0.1:8080/

      Show
      Johnathan Meehan added a comment - - edited > Regarding the "max-age", I know that for now, some browsers ignore it. > But effectively we can provide a settter with seconds, and still use the property "expires", or something like that Good point, and that's a nice idea. Let's roll with that. > I also tried to follow the new rfc6265, which makes obsolete the property "discard" EDIT: Removed reference to wrong spec. Sorry, Michele. Let's run with your work, Michele - it's fast and ready-to-go. Could you put a final version together for review, with tests, documentation, the fake max-age method and anything else you like? Ping me outside of JIRA if you are short on time and I'll be glad to help out. > What do you use for your benchmarks?! ab -k -c10 -n250000 http://127.0.0.1:8080/
      Hide
      Michele Zuccalà added a comment -

      Johnathan,

      I started working.
      If I have time, I post also my implementation of signed cookies
      thanks

      Show
      Michele Zuccalà added a comment - Johnathan, I started working. If I have time, I post also my implementation of signed cookies thanks
      Hide
      Michele Zuccalà added a comment -

      I've attached final version of my patch.
      The patch don't contains my implementation of signed cookie, because I've used the following class for Base64

      http://iharder.sourceforge.net/current/java/base64/

      and I don't know if this implementation can be included without problems.
      Thanks

      Show
      Michele Zuccalà added a comment - I've attached final version of my patch. The patch don't contains my implementation of signed cookie, because I've used the following class for Base64 http://iharder.sourceforge.net/current/java/base64/ and I don't know if this implementation can be included without problems. Thanks
      Michele Zuccalà made changes -
      Attachment deft-52bis.patch [ 12488244 ]
      Hide
      Johnathan Meehan added a comment - - edited

      Hey, Michele. Nicely done!
      I've made a couple of tiny changes, like adding the ASF header and type-level Javadoc (DEFT-166). Please take a look and make sure it's still okay, and that you are happy with things (deft-52bis-v2.patch).
      Perhaps post to deft-dev with your question about licensing? The comment he makes, "...you can do whatever you want with it. Really..." might be enough, but a mentor would have a better answer.

      Show
      Johnathan Meehan added a comment - - edited Hey, Michele. Nicely done! I've made a couple of tiny changes, like adding the ASF header and type-level Javadoc ( DEFT-166 ). Please take a look and make sure it's still okay, and that you are happy with things (deft-52bis-v2.patch). Perhaps post to deft-dev with your question about licensing? The comment he makes, "...you can do whatever you want with it. Really..." might be enough, but a mentor would have a better answer.
      Johnathan Meehan made changes -
      Attachment deft-52bis-v2.patch [ 12488253 ]
      Hide
      Michele Zuccalà added a comment -

      I'm glad you liked my work and your changes are ok for me

      about Base64 class, the author also says

      "You do not have to match it up with Any Other open-source license, just use it. You can rename the files, move the Java packages, whatever you want"

      then, I could easily reuse the code, but I'd like to eventually add it and leave the author credits
      So take your advice and ask on deft-dev

      Show
      Michele Zuccalà added a comment - I'm glad you liked my work and your changes are ok for me about Base64 class, the author also says "You do not have to match it up with Any Other open-source license, just use it. You can rename the files, move the Java packages, whatever you want" then, I could easily reuse the code, but I'd like to eventually add it and leave the author credits So take your advice and ask on deft-dev
      Hide
      Johnathan Meehan added a comment -

      +1 (deft-52bis-v2.patch)

      Show
      Johnathan Meehan added a comment - +1 (deft-52bis-v2.patch)
      Johnathan Meehan made changes -
      Attachment deft-52bis-v2.patch [ 12488256 ]
      Johnathan Meehan made changes -
      Attachment deft-52bis-v2.patch [ 12488253 ]
      Michele Zuccalà made changes -
      Attachment deft-52.patch [ 12487812 ]
      Michele Zuccalà made changes -
      Attachment deft-52bis.patch [ 12488244 ]
      Hide
      Emmanuel Lecharny added a comment -

      Patch applied in sandbox.

      Show
      Emmanuel Lecharny added a comment - Patch applied in sandbox.
      Hide
      Emmanuel Lecharny added a comment -
      Show
      Emmanuel Lecharny added a comment - Patch applied : http://svn.apache.org/viewvc?rev=1152427&view=rev
      Emmanuel Lecharny made changes -
      Status In Progress [ 3 ] Resolved [ 5 ]
      Resolution Fixed [ 1 ]
      Johnathan Meehan made changes -
      Affects Version/s 0.4.0 [ 12317348 ]
      Hide
      Johnathan Meehan added a comment -

      Patch applied; closing.

      Show
      Johnathan Meehan added a comment - Patch applied; closing.
      Johnathan Meehan made changes -
      Status Resolved [ 5 ] Closed [ 6 ]
      Hide
      Roger Schildmeijer added a comment -

      affects version? shouldn't it be "fix version"?

      Show
      Roger Schildmeijer added a comment - affects version? shouldn't it be "fix version"?
      Hide
      Johnathan Meehan added a comment -

      It should; let's sort that out now.

      Show
      Johnathan Meehan added a comment - It should; let's sort that out now.
      Johnathan Meehan made changes -
      Resolution Fixed [ 1 ]
      Status Closed [ 6 ] Reopened [ 4 ]
      Johnathan Meehan made changes -
      Affects Version/s 0.4.0 [ 12317348 ]
      Fix Version/s 0.4.0 [ 12317348 ]
      Johnathan Meehan made changes -
      Status Reopened [ 4 ] Resolved [ 5 ]
      Resolution Fixed [ 1 ]
      Johnathan Meehan made changes -
      Status Resolved [ 5 ] Closed [ 6 ]
      Tony Stevenson made changes -
      Project Deft [ 12311521 ] Apache AWF [ 12313220 ]
      Key DEFT-52 AWF-157
      Reporter Niklas Gustavsson [ niklas ]
      Issue Type New Feature [ 2 ] Bug [ 1 ]
      Fix Version/s 0.4.0 [ 12317348 ]

        People

        • Assignee:
          Johnathan Meehan
          Reporter:
          Niklas Gustavsson
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development