Skip to content

Develop#223

Merged
aleks-f merged 2 commits intopocoproject:developfrom
karlr42:develop
Jun 17, 2013
Merged

Develop#223
aleks-f merged 2 commits intopocoproject:developfrom
karlr42:develop

Conversation

@karlr42
Copy link
Copy Markdown
Contributor

@karlr42 karlr42 commented Jun 17, 2013

Make HTTPCookie support expiry times in the past. Fix #222

The testcase passes fine for me on Linux-gcc, but the code probably isn't great. Specifically I'm not sure why the constructor for HTTPCookie seems to (consistently) subtract one second from timestamps in the future. I worked around it though.

aleks-f added a commit that referenced this pull request Jun 17, 2013
I'll check and run it on other platforms/compilers before 1.5.2 release
@aleks-f aleks-f merged commit d5ec46b into pocoproject:develop Jun 17, 2013
@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Jun 18, 2013

Note: it's not that the constructor subtracts a second, it is simply about timing when toString() is called; both constructor and toString() take a timestamp, while _maxAge remains the same. So, the string generated by toString() call will "drift" into the future with the lifetime of the cookie. I'm not sure if this is good design but that's how the class behaves now. To me it seems that the absolute "expires" was much more clear but, if I'm not mistaken, that's obsolete ...

@obiltschnig
Copy link
Copy Markdown
Member

I guess the problem with the absolute expire time was that it may lead to issues for very short expire times when the clocks of client and server drift too much.
In the test case, the hardcoded 1 second drift makes the test a bit brittle. Proper fix would be to parse out the date/time from the string and compare against expected value with a delta.

@karlr42
Copy link
Copy Markdown
Contributor Author

karlr42 commented Jun 18, 2013

Ok, thanks for clarifying. Sure, that sounds better. I will change the testcase to time the elapsed time between creating the cookie and calling toString and then use that delta in a fuzzy comparison with the expected value.

@karlr42
Copy link
Copy Markdown
Contributor Author

karlr42 commented Jun 18, 2013

Commit 5a70971 (real changes in d3a61a0)

I changed the testcase to be nicer and less brittle for now. Idea is to pass an expiry time string into the cookie on creation, call toString, time how long the cookie was alive between those two events, then use that delta in a comparison between the given expiry information and the expiry information printed by toString in both v0 and v1 modes. So I think that matches "parse out the date/time from the string and compare against expected value with a delta."

As an aside, it might be nice to have overloads of get/setMaxAge which take DateTimes/Timestamps to make it more intuitive for applications to create a cookie with a specific expiry date(for web applications trying to generate session-cookie-expiring Set-Cookie headers, or trying to set the length of a login session). I can do that, but I'm busy with my own projects at the moment so might not be in time for 1.5.2.

@karlr42
Copy link
Copy Markdown
Contributor Author

karlr42 commented Jun 18, 2013

Apologies Aleksandar, I see you cleaned up my old test case. I'll merge and fix up the style according to how you have it and submit a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTPCookie doesn't support expiry times in the past

3 participants