route_cache breaks with multiple parameters
Hello, I've opened bug report 57158 where the following code: set route_cache => 1; get '/number/:cluster/:category' => sub { # your code here }; will provide incorrect params after a few attempts. I have had the same issue earlier with code like this: get '/number/:cluster/*' => sub { # your code here }; using splat(), but at the time I thought I had missed something in the documentation. I suspect this might be due to a similar problem. Running with route_cache: 0 will work around the issue. HTH, Stéphane
On Mon, May 3, 2010 at 1:18 PM, Stéphane <stephane@shimaore.net> wrote:
Hello,
I've opened bug report 57158
Thank you. Would it be possible for you to sum it up in a test script? That would be really helpful to make sure once it's fixed, it won't happen again. Sawyer.
I've opened bug report 57158 Would it be possible for you to sum it up in a test script?
Finally found some time for it. I attached the test script to the ticket. However while writing it I ran into a separate issue (with or without route_cache): # The following works fine get '/beep1/*' => sub { "Splat: ".join(',',splat()); }; get '/beep1/*/*' => sub { "Splat: ".join(',',splat()); }; # The following breaks get '/beep2/:name/*' => sub { "Name: ".params->{name}." splat: ".join(',',splat()); }; So mixing named parameters and splat fails. Before I report this (as a separate issue), is this a "supported" way of doing things? Stéphane
Hey Stephane, On Wed, May 19, 2010 at 12:16 AM, Stéphane <stephane@shimaore.net> wrote:
I've opened bug report 57158
Would it be possible for you to sum it up in a test script?
Finally found some time for it.
I attached the test script to the ticket.
This has been incredibly helpful, thank you! It seems like the match() function is matching /name/bob and /name/bill and changing the parameter of :name to "bill" from "bob". Since we're working with references here and the cache uses references to keep track, it means that it affects the cache as well. When it tries to store "/name/bill" as a new value, it sees that the reference to "/name/bob" now points to whatever "/name/bill" is suppose to so it aliases the key of "bill" in the cache to that of "bob". I'm having a bit of a hard time explaining (but it's 3:30AM so you'll forgive me). Basically what it means is: - Cache: "I'm storing Bob now..." - Route matching: "I see Bill, but it's the same match as with Bob so I'll just change Bob's parameters to Bill's" - Cache: "I'm suppose to store Bill now, but since what was Bob is now the same as Bill, I'll just alias it" - User: "Get me Bob.. wait... this is Bill!" - Developers: "Damn it!" I'll hack on it a bit tomorrow and keep you posted.
However while writing it I ran into a separate issue (with or without route_cache):
# The following works fine
get '/beep1/*' => sub { "Splat: ".join(',',splat()); };
get '/beep1/*/*' => sub { "Splat: ".join(',',splat()); };
# The following breaks
get '/beep2/:name/*' => sub { "Name: ".params->{name}." splat: ".join(',',splat()); };
So mixing named parameters and splat fails. Before I report this (as a separate issue), is this a "supported" way of doing things?
Better report it as a separate issue. Worst case is it won't be fixed if enough developers think it shouldn't be mixed. Even in that case, changes will be required at least in the docs to make it clear. Always good to open an issue. :) Thanks again, Sawyer.
On Wed, May 19, 2010 at 3:30 AM, sawyer x <xsawyerx@gmail.com> wrote:
It seems like the match() function is matching /name/bob and /name/bill and changing the parameter of :name to "bill" from "bob". Since we're working with references here and the cache uses references to keep track, it means that it affects the cache as well. When it tries to store "/name/bill" as a new value, it sees that the reference to "/name/bob" now points to whatever "/name/bill" is suppose to so it aliases the key of "bill" in the cache to that of "bob".
More digging shows that match() probably matches a path to a route reference and then just changes the content from the request and passes it along. This worked out just fine for the past but with caching and references this gets you a race condition that cannot be helped. I was able to fix it by creating a reference to a new hash, copying the details and continuing along with it. However, this caused a few tests to fail, so tomorrow I'll go over the failing tests and if I can resolve everything, I'll commit the changes and close the ticket. Again, thank you for the attention and test! :) Sawyer.
On Wed, May 19, 2010 at 3:39 AM, sawyer x <xsawyerx@gmail.com> wrote:
I was able to fix it by creating a reference to a new hash, copying the details and continuing along with it. However, this caused a few tests to fail, so tomorrow I'll go over the failing tests and if I can resolve everything, I'll commit the changes and close the ticket.
You'll be happy to know this issue was resolved (as reflected in RT as well) and I'll be sending an announcement on this. Your test was integrated into Dancer to avoid this from happening again in the future. Thanks! Sawyer.
On 18/05/2010 23:16, Stéphane wrote:
get '/beep2/:name/*' => sub { "Name: ".params->{name}." splat: ".join(',',splat()); };
So mixing named parameters and splat fails. Before I report this (as a separate issue), is this a "supported" way of doing things?
No, this is not supported, and I'm quite sure I've written so in the documentation... somewhere :) But I can't remember where exactly. -- Alexis Sukrieh
So mixing named parameters and splat fails. Before I report this (as a separate issue), is this a "supported" way of doing things? No, this is not supported, and I'm quite sure I've written so in the documentation... somewhere :) But I can't remember where exactly.
Sorry -- I asked a lazy question. About I rephrase it as: would it be OK to support it? (See attached patch.) Stéphane
I added test cases to the patch and posted it as a wishlist item on RT.cpan (#57720). S.
participants (3)
-
Alexis Sukrieh -
sawyer x -
Stéphane