fix parsed cookies cache inside $cgi->cookie(...)

[bambr]

Benchmark:

$ HTTP_COOKIE="cookie1=value1; cookie2=value2; cookie3=value3" \
> perl -MCGI -MBenchmark -e'my $cgi = CGI->new; timethis(-1, sub { $cgi->cookie("cookie2") })'

Benchmark results:

timethis for 1:  1 wallclock secs ( 1.06 usr +  0.00 sys =  1.06 CPU) @ 77283.02/s (n=81920) # new
timethis for 1:  1 wallclock secs ( 1.13 usr +  0.01 sys =  1.14 CPU) @ 8274.56/s (n=9433)   # old

Fixes #251

[leejo]

Looks good, but can you add some test coverage that confirms the bug before and the fix after?

[bambr]

Done. This test relies that we use CGI::Cookie->fetch inside cookie method. This doesn't look good for me, but I don't have another idea to cover this. If you have one, I can try to change the test.

[leejo]

Done. This test relies that we use CGI::Cookie->fetch inside cookie method. This doesn't look good for me, but I don't have another idea to cover this. If you have one, I can try to change the test.

Looks fine to me.

I'm a little hesitant on this change however. We have, effectively, a cache that was introduced a long time ago (predates the git repo at least, which started in 2009) but appears to have not been working since then. So if we now fix that cache to do what it was originally intended to do we might cause a problem to any code that has been calling CGI->cookie, fiddling the request headers or ENV variables, and then calling CGI->cookie again.

This is not something I would hope is happening in people's production code, but I would be totally unsurprised to find out someone/thing is doing that, and this change may break their code. I would expect to see this more in test code. But anyway, yeah I'm not sure about this. It may be better to have this caching feature fix enabled by way of a flag, much like other features in the CGI.pm module?

[bambr]

Yes, you are right, this is possible. Flags are $UNLINK_TMP_FILES, $DISABLE_UPLOADS etc.?

[leejo]

Yes, you are right, this is possible. Flags are $UNLINK_TMP_FILES, $DISABLE_UPLOADS etc.?

That's correct, yep!

[bambr]

Done

[leejo]

Looks good 👍 - will get this out to CPAN sometime in the next week or so. Thanks for the patch!

[bambr]

Thank you!

P.S. Never thought I will fix CGI.pm which I never used :) This bug was found in legacy project using NYTProf top.

[leejo]

v4.54 on its way to CPAN. Thanks again 👍