Shiro
  1. Shiro
  2. SHIRO-226

Default rememberMe cookie size is rather large

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.1.0
    • Fix Version/s: None
    • Component/s: RememberMe
    • Labels:
      None

      Description

      The rememberMe cookie size is fairly big since it uses the DefaultSerializer, which serializes a lot of stuff which is not really necessary - especially for the fairly common single realm - single principal case. Please see discussion at http://mail-archives.apache.org/mod_mbox/shiro-user/201012.mbox/%3C2C685FDE-71EE-4834-8816-8E00C890D050@ecyrd.com%3E

        Activity

        Janne Jalkanen created issue -
        Hide
        Janne Jalkanen added a comment -

        The included patch proposes a SimplePrincipalSerializer, which provides significant size benefits without (hopefully) sacrificing genericity.

        The results are as follows (can be replicated by running the included test case).

        Single principal, single realm
        Default serializer, Simple serializer, Size saving
                       421                100       76.25%
        
        Multiple principals, single realm
        Default serializer, Simple serializer, Size saving
                       575                254       55.83%
        
        Multiple principals, multiple realms
        Default serializer, Simple serializer, Size saving
                       815                368       54.85%
        
        Show
        Janne Jalkanen added a comment - The included patch proposes a SimplePrincipalSerializer, which provides significant size benefits without (hopefully) sacrificing genericity. The results are as follows (can be replicated by running the included test case). Single principal, single realm Default serializer, Simple serializer, Size saving 421 100 76.25% Multiple principals, single realm Default serializer, Simple serializer, Size saving 575 254 55.83% Multiple principals, multiple realms Default serializer, Simple serializer, Size saving 815 368 54.85%
        Janne Jalkanen made changes -
        Field Original Value New Value
        Attachment SHIRO-226-jalkanen-1.patch [ 12466314 ]
        Hide
        Janne Jalkanen added a comment -

        Oops, patch is against r1049525, i.e. current trunk.

        Show
        Janne Jalkanen added a comment - Oops, patch is against r1049525, i.e. current trunk.
        Hide
        Kalle Korhonen added a comment -

        Thanks Janne! As is, the patch provides an alternative to the DefaultSerializer, thereby rolling the responsibility of using it (and first finding out a problem and solution exists!) to the users which is not ideal. But perhaps we shouldn't worry about that as part of this issue; the patch itself looks good and I don't have any objections to including it.

        Show
        Kalle Korhonen added a comment - Thanks Janne! As is, the patch provides an alternative to the DefaultSerializer, thereby rolling the responsibility of using it (and first finding out a problem and solution exists!) to the users which is not ideal. But perhaps we shouldn't worry about that as part of this issue; the patch itself looks good and I don't have any objections to including it.
        Hide
        Janne Jalkanen added a comment -

        Yup, I specifically did not wish to mess with DefaultSerializer because changing it would mean breaking everyone's rememberMe -cookies - and because there might be other uses for DefaultSerializer as well. I'm leaving the overall impact analysis to you guys

        Show
        Janne Jalkanen added a comment - Yup, I specifically did not wish to mess with DefaultSerializer because changing it would mean breaking everyone's rememberMe -cookies - and because there might be other uses for DefaultSerializer as well. I'm leaving the overall impact analysis to you guys
        Hide
        Les Hazlewood added a comment -

        Hi Janne,

        This is good - thanks very much for the patch!

        Before we resolve this issue though, I think the default SimplePrincipalCollection should contain this logic as well - there are times when principals are serialized outside the context of 'rememberMe' (e.g when caching), and those usages should benefit from this as well.

        Also, this would further reduce the need for users to manually specify the SimplePrincipalSerializer, since the size savings between it and the default serialization approach should be very small (the difference being the number of bytes taken up by the MAGIC constant vs the class name included by the JDK).

        I'll implement those changes in SimplePrincipalCollection, create some more tests for it, and update this issue when I'm done.

        Show
        Les Hazlewood added a comment - Hi Janne, This is good - thanks very much for the patch! Before we resolve this issue though, I think the default SimplePrincipalCollection should contain this logic as well - there are times when principals are serialized outside the context of 'rememberMe' (e.g when caching), and those usages should benefit from this as well. Also, this would further reduce the need for users to manually specify the SimplePrincipalSerializer, since the size savings between it and the default serialization approach should be very small (the difference being the number of bytes taken up by the MAGIC constant vs the class name included by the JDK). I'll implement those changes in SimplePrincipalCollection, create some more tests for it, and update this issue when I'm done.
        Hide
        Les Hazlewood added a comment -

        I just thought of something as well - we can try to use a Zip stream to compress the output even more. I'll try that in some of our tests and see what happens w/ regards to time vs space.

        Show
        Les Hazlewood added a comment - I just thought of something as well - we can try to use a Zip stream to compress the output even more. I'll try that in some of our tests and see what happens w/ regards to time vs space.

          People

          • Assignee:
            Unassigned
            Reporter:
            Janne Jalkanen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development