code review 6846125: exp/cookiejar: implement MemStorage.

exp/cookiejar: implement MemStorage.

Update issue 1960.

[nigeltao, 2012-11-29 04:52:39]

Hello bradfitz@golang.org, dr.volker.dobler@gmail.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

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

[volker.dobler, 2012-11-29 13:37:26]

I am feeling more and more uncomfortable with the
current Storage, I am afraid it might be at the
border of becoming javaesque over-engineered with
too many abstractions and generalizations just
because one might want to do something without an
actual need to do so. 

Counter-proposal: http://codereview.appspot.com/6854114/
based on your counter-proposal.

Rational:
The API of Storage should serve the needs of
Cookies and SetCookies well, while being simple to
implement. Thus: How about disallowing the filter
functions in Entries and Keys to call other methods?
What exactly would be lost, except generality?
A generality which won't be used anyway?

If we take the idea of a dumb, simple to implement,
storage abstraction serious we should drop all this
fanciness, maybe not even keeping LastAccess.
This would make cookie retrieval much slower, but
that might be fine: For an all in-memory storage it
might still be much faster than the actual HTTP
request and disc or DB based storages will need a
memory cache anyway to be useful.

If this is seems too radical I see several valid
options:
 - Expose full cookie life-cycle in Entry, that is
   Creation, LastAccess, and Expires and require
   Storage to handle the life-cycle of a cookie
   properly. (Which borders at the last option...)
 - Make memStorage a "friend of Jar" with knowledge
   about an entry's data internals and typeswitch 
   in Jar if its Storage is a memStorage and have
   memStorage do more or extra work via additonal
   methods. (A hack.)
 - Do as browsers do: Keep cookies in memory and
   asynchronously inform a backend about changes.
   (I still find this attractive. Any counter-arguments?)

https://codereview.appspot.com/6846125/diff/5001/src/pkg/exp/cookiejar/storag...
File src/pkg/exp/cookiejar/storage.go (right):

https://codereview.appspot.com/6846125/diff/5001/src/pkg/exp/cookiejar/storag...
src/pkg/exp/cookiejar/storage.go:51: // f may call a Storage's Add and Delete
methods. Modification during
I always wondered why? Do you have a clever implementation
for Jar lying around somewhere which benefits from allowing
f to call other methods?

Personally I'd prefer to disallow f to call any other
methods and keep Entries and Keys simple. If this
complicates other functions except SetCookies and Cookies:
I am fine with that. E.g. a two pass scanning with
calling Entries twice seems okay for me in specialized
methods of Jar or functions operating on Storage like
e.g. limiting numbers of cookies stored.

https://codereview.appspot.com/6846125/diff/5001/src/pkg/exp/cookiejar/storag...
src/pkg/exp/cookiejar/storage.go:153: entry.Creation = old.Creation
That is broken if old is an expired cookie: If old is
expired you should actually set a new cookie with
a new Creation time.
Possible solutions:
A) Remove Creation from Entry (and encode in Data).
   Scan m.m[key] first with Entries and find the
   proper Creation time when adding/updating cookies
   from within SetCookies
   Pros: Entry gets opaquer. Cons: Two passes needed
B) Move Expires to Entry.
   Require Add to check Expires before reusing old
   Creation time.
   Pros: No need for SetCookies to scan first.
   Cons: More logic and potential bugs in types
   implementing Storage

[bradfitz, 2012-11-29 19:50:41]

I agree that this storage API is way more complicated than I was expecting
to find when I opened the CL.

I haven't been following the last few rounds of email on this, though, so
I'm not sure which requirements dictate its complexity.

On Wed, Nov 28, 2012 at 8:52 PM, <nigeltao@golang.org> wrote:

> Reviewers: bradfitz, volker.dobler,
>
> Message:
> Hello bradfitz@golang.org, dr.volker.dobler@gmail.com (cc:
> golang-dev@googlegroups.com, rsc@golang.org),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> exp/cookiejar: implement MemStorage.
>
> Update issue 1960.
>
> Please review this at
http://codereview.appspot.com/**6846125/<http://codereview.appspot.com/6846125/>
>
> Affected files:
>   M src/pkg/exp/cookiejar/storage.**go
>   M src/pkg/exp/cookiejar/storage_**test.go
>
>
>

[nigeltao, 2012-12-21 00:17:39]

On Fri, Nov 30, 2012 at 6:50 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I agree that this storage API is way more complicated than I was expecting
> to find when I opened the CL.

I have yet another proposal, this time one in which Storage is very
dumb and only deals with opaque []byte values. A Jar primarily keeps
its cookies in memory; it's been a while since we had this
conversation but I believe that Volker would describe this as similar
to what Chromium / Firefox does:
https://codereview.appspot.com/6996044

[nigeltao, 2013-01-29 00:36:20]

*** Abandoned ***

[nigeltao, 2013-02-01 00:54:02]

*** Abandoned ***