code review 7323063: exp/cookiejar: replace fake creation time by order parameter (issue 7323063)

[dr.volke...@gmail.com]

Reviewers: nigeltao,

Message:
Hello nige...@golang.org (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/


Description:
exp/cookiejar: replace fake creation time by order parameter

Sorting of cookies with same path length and same creation
time is done by an additional order field.
This makes the order in which cookies are returned in Cookies
deterministic, even if the system clock is manipulated.

Please review this at https://codereview.appspot.com/7323063/

Affected files:
M src/pkg/exp/cookiejar/jar.go


Index: src/pkg/exp/cookiejar/jar.go
===================================================================
--- a/src/pkg/exp/cookiejar/jar.go
+++ b/src/pkg/exp/cookiejar/jar.go
@@ -78,7 +78,7 @@
}

// entry is the internal representation of a cookie.
-// The fields are those of RFC 6265.
+// The upper case fields are those of RFC 6265.
type entry struct {
Name string
Value string
@@ -91,6 +91,7 @@
Expires time.Time
Creation time.Time
LastAccess time.Time
+ order int // sequence number of cookies set in one Set-Cookie header
}

// Id returns the domain;path;name triple of e as an id.
@@ -137,7 +138,18 @@
func (s byPathLength) Less(i, j int) bool {
in, jn := len(s[i].Path), len(s[j].Path)
if in == jn {
- return s[i].Creation.Before(s[j].Creation)
+ if s[i].Creation.Before(s[j].Creation) {
+ return true
+ } else if s[i].Creation.After(s[j].Creation) {
+ return false
+ }
+ // Sorting cookies with equal creation time according to their
+ // order. This makes the order of cookies received in a single
+ // Set-Cookie header deterministic.
+ if s[i].order < s[j].order {
+ return true
+ }
+ return false
}
return in > jn
}
@@ -228,6 +240,7 @@
now := time.Now()

modified := false
+ order := 0
for _, cookie := range cookies {
e, remove, err := j.newEntry(cookie, now, defPath, host)
if err != nil {
@@ -249,16 +262,15 @@

if old, ok := submap[id]; ok {
e.Creation = old.Creation
+ e.order = old.order
} else {
e.Creation = now
+ e.order = order
}
e.LastAccess = now
submap[id] = e
modified = true
- // Make Creation and LastAccess strictly monotonic forcing
- // deterministic behaviour during sorting.
- // TODO: check if this is conforming to RFC 6265.
- now = now.Add(1 * time.Nanosecond)
+ order++
}

if modified {

[dr.volke...@gmail.com]

Hello nige...@golang.org (cc: golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/7323063/

[alex.b...@gmail.com]

Still fails on windows:

=== RUN TestCanonicalHost
--- PASS: TestCanonicalHost (0.00 seconds)
=== RUN TestHasPort
--- PASS: TestHasPort (0.00 seconds)
=== RUN TestJarKey
--- PASS: TestJarKey (0.00 seconds)
=== RUN TestIsIP
--- PASS: TestIsIP (0.00 seconds)
=== RUN TestDefaultPath
--- PASS: TestDefaultPath (0.00 seconds)
=== RUN TestDomainAndType
--- PASS: TestDomainAndType (0.00 seconds)
=== RUN TestBasics
--- PASS: TestBasics (0.00 seconds)
=== RUN TestUpdateAndDelete
--- FAIL: TestUpdateAndDelete (0.00 seconds)
jar_test.go:266: Test "Delete all." Content
got "b=2"
want ""
jar_test.go:276: Test "Delete all." #0
got "b=2"
want ""
jar_test.go:266: Test "Refill #1." Content
got "A=1 A=2 A=3 A=4 b=2"
want "A=1 A=2 A=3 A=4"
jar_test.go:276: Test "Refill #1." #0
got "A=2 A=4 b=2 A=1 A=3"
want "A=2 A=4 A=1 A=3"
jar_test.go:266: Test "Refill #2." Content
got "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9 b=2"
want "A=1 A=2 A=3 A=4 A=6 A=7 A=8 A=9"
jar_test.go:276: Test "Refill #2." #0
got "A=2 A=4 b=2 A=1 A=3"
want "A=2 A=4 A=1 A=3"
jar_test.go:266: Test "Delete A7." Content
got "A=1 A=2 A=3 A=4 A=6 A=8 A=9 b=2"
want "A=1 A=2 A=3 A=4 A=6 A=8 A=9"
jar_test.go:276: Test "Delete A7." #0
got "A=2 A=4 b=2 A=1 A=3"
want "A=2 A=4 A=1 A=3"
jar_test.go:266: Test "Delete A4." Content
got "A=1 A=2 A=3 A=6 A=8 A=9 b=2"
want "A=1 A=2 A=3 A=6 A=8 A=9"
jar_test.go:276: Test "Delete A4." #0
got "A=2 b=2 A=1 A=3"
want "A=2 A=1 A=3"
jar_test.go:266: Test "Delete A6." Content
got "A=1 A=2 A=3 A=8 A=9 b=2"
want "A=1 A=2 A=3 A=8 A=9"
jar_test.go:276: Test "Delete A6." #0
got "A=2 b=2 A=1 A=3"
want "A=2 A=1 A=3"
jar_test.go:266: Test "Delete A3." Content
got "A=1 A=2 A=8 A=9 b=2"
want "A=1 A=2 A=8 A=9"
jar_test.go:276: Test "Delete A3." #0
got "A=2 b=2 A=1"
want "A=2 A=1"
jar_test.go:266: Test "No cross-domain delete." Content
got "A=1 A=2 A=8 A=9 b=2"
want "A=1 A=2 A=8 A=9"
jar_test.go:276: Test "No cross-domain delete." #0
got "A=2 b=2 A=1"
want "A=2 A=1"
jar_test.go:266: Test "Delete A8 and A9." Content
got "A=1 A=2 b=2"
want "A=1 A=2"
jar_test.go:276: Test "Delete A8 and A9." #0
got "A=2 b=2 A=1"
want "A=2 A=1"
=== RUN TestExpiration
--- FAIL: TestExpiration (1.50 seconds)
jar_test.go:266: Test "Check jar." Content
got "a=1 c=3 d=4"
want "a=1 d=4"
jar_test.go:276: Test "Check jar." #0
got "a=1 c=3 d=4"
want "a=1 d=4"
=== RUN TestChromiumBasics
--- PASS: TestChromiumBasics (0.00 seconds)
=== RUN TestChromiumDomain
--- PASS: TestChromiumDomain (0.00 seconds)
=== RUN TestChromiumDeletion
--- FAIL: TestChromiumDeletion (0.00 seconds)
jar_test.go:266: Test "Delete sc b2 via Expires." Content
got "b=2"
want ""
jar_test.go:276: Test "Delete sc b2 via Expires." #0
got "b=2"
want ""
jar_test.go:266: Test "Create persistent cookie c3." Content
got "b=2 c=3"
want "c=3"
jar_test.go:276: Test "Create persistent cookie c3." #0
got "b=2 c=3"
want "c=3"
jar_test.go:266: Test "Delete pc c3 via MaxAge." Content
got "b=2"
want ""
jar_test.go:276: Test "Delete pc c3 via MaxAge." #0
got "b=2"
want ""
jar_test.go:266: Test "Create persistent cookie d4." Content
got "b=2 d=4"
want "d=4"
jar_test.go:276: Test "Create persistent cookie d4." #0
got "b=2 d=4"
want "d=4"
jar_test.go:266: Test "Delete pc d4 via Expires." Content
got "b=2 d=4"
want ""
jar_test.go:276: Test "Delete pc d4 via Expires." #0
got "b=2 d=4"
want ""
FAIL

Alex

https://codereview.appspot.com/7323063/

[nige...@golang.org]

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode68
src/pkg/exp/cookiejar/jar.go:68: cnt uint64
I'd call the field nextSeqNum.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode84
src/pkg/exp/cookiejar/jar.go:84: // The upper case fields are those of
RFC 6265.

// entry is the internal representation of a cookie.

//
// This struct type is not used outside of this package per se, but the
exported
// fields are those of RFC 6265.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode97
src/pkg/exp/cookiejar/jar.go:97: order uint64 // sequence number of

cookies set in one Set-Cookie header

I'd write
LastAccess time.Time

// seqNum is a sequence number so that Cookies returns cookies in a
// deterministic order, even for cookies that have equal Path length and
// equal Creation time. This simplifies testing.
seqNum uint64

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode142
src/pkg/exp/cookiejar/jar.go:142: in, jn := len(s[i].Path),
len(s[j].Path)
How about

if len(s[i].Path) != len(s[j].Path) {
return len(s[i].Path) > len(s[j].Path)
}
if !s[i].Creation.Equal(s[j].Creation) {

return s[i].Creation.Before(s[j].Creation)
}

return s[i].seqNum < s[j].seqNum

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode270
src/pkg/exp/cookiejar/jar.go:270: e.order = j.cnt
e.seqNum = j.nextSeqNum
j.nextSeqNum++

https://codereview.appspot.com/7323063/

[dr.volke...@gmail.com]

PTAL

Incorporated all the feedback and tried to make the
expiration code more stable by:
* replacing "expires < now" by "!(expires > now)"
* increasing some deltas
* waiting 1.6 seconds for expiration as rounding may
create Expires 1.5 seconds in the future.

I did some tests with a complete fake clock which reports
the same time instance and the deletion tests pass.
So this should fix the Windows build.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode68
src/pkg/exp/cookiejar/jar.go:68: cnt uint64

On 2013/02/15 04:43:01, nigeltao wrote:
> I'd call the field nextSeqNum.

Done.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode84
src/pkg/exp/cookiejar/jar.go:84: // The upper case fields are those of
RFC 6265.

On 2013/02/15 04:43:01, nigeltao wrote:
> // entry is the internal representation of a cookie.
> //
> // This struct type is not used outside of this package per se, but
the exported
> // fields are those of RFC 6265.

Done.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode97
src/pkg/exp/cookiejar/jar.go:97: order uint64 // sequence number of
cookies set in one Set-Cookie header

On 2013/02/15 04:43:01, nigeltao wrote:
> I'd write
> LastAccess time.Time

> // seqNum is a sequence number so that Cookies returns cookies in a
> // deterministic order, even for cookies that have equal Path length
and
> // equal Creation time. This simplifies testing.
> seqNum uint64

Done.

I kept the "simplifies testing" even if I think that the
deterministic order is required by RFC 6265.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode142
src/pkg/exp/cookiejar/jar.go:142: in, jn := len(s[i].Path),
len(s[j].Path)

On 2013/02/15 04:43:01, nigeltao wrote:
> How about

> if len(s[i].Path) != len(s[j].Path) {
> return len(s[i].Path) > len(s[j].Path)
> }
> if !s[i].Creation.Equal(s[j].Creation) {
> return s[i].Creation.Before(s[j].Creation)
> }
> return s[i].seqNum < s[j].seqNum

Done.

https://codereview.appspot.com/7323063/diff/12001/src/pkg/exp/cookiejar/jar.go#newcode270
src/pkg/exp/cookiejar/jar.go:270: e.order = j.cnt

On 2013/02/15 04:43:01, nigeltao wrote:
> e.seqNum = j.nextSeqNum
> j.nextSeqNum++

Done.

https://codereview.appspot.com/7323063/

[alex.b...@gmail.com]

Still fails on windows:

--- FAIL: TestExpiration (1.60 seconds)

jar_test.go:266: Test "Check jar." Content
got "a=1 c=3 d=4"
want "a=1 d=4"
jar_test.go:276: Test "Check jar." #0
got "a=1 c=3 d=4"
want "a=1 d=4"

FAIL exp/cookiejar 2.240s


https://codereview.appspot.com/7323063/

[Volker Dobler]

That is strange. I cannot reproduce the failure on

Windows 64.

Could you try it once more with 1600 replaced by 10000 

in jar_test.go:612 just to see if it is a timing issue or

something fundamental.

Thanks in advance.

--
Dr. Volker Dobler

[Dave Cheney]

Time inside vm's is strange.

> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[dr.volke...@gmail.com]

PTAL

Started to remove expiration tests based on passing time
by artificially creating expired cookies and checking
internal state.


https://codereview.appspot.com/7323063/