-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cookiejar implementation. #526
base: master
Are you sure you want to change the base?
Conversation
…d CookieJar structure helping user to collect cookies easily without using external packages.
I think if we're going to add a CookieJar we should do it right and follow the spec. So:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to handle cookies expiring and paths.
…y URI. Added function to release cookies by expiration. Added function to copy cookie slices
…ged getFrom and dumpTo functions
cookiejar.go
Outdated
c := searchCookieByKeyAndPath(key, path, cookies) | ||
if c == nil { | ||
c = AcquireCookie() | ||
created = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better do the expired check here first so you don't acquire and release a cookie immediately if it didn't exist already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any other way to do this.
If I do the expiration checking at cookiejar.go:197 first I need to call ParseBytes and the code will be repeated in the case c == nil
and c != nil
. I prefer to keep it like this unless you see any other way to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right, I forgot that you have to somehow parse the cookie first to get the expiration date. Doing that in a stack allocate cookie value would be possible for example but it would result in messier code.
cookiejar.go
Outdated
cj.m.Lock() | ||
{ | ||
if cj.hostCookies == nil { | ||
cj.hostCookies = make(map[string][]*Cookie) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can just do return
. No need to allocate anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make sure you do defer cj.m.Unlock()
instead of doing this at the end of the function.
I myself always prefer to defer
Unlock
s as its much easier to follow the code. Also defer
is basically free these days so no worry about performance.
cookiejar.go
Outdated
c := searchCookieByKeyAndPath(key, path, cookies) | ||
if c == nil { | ||
c = AcquireCookie() | ||
created = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right, I forgot that you have to somehow parse the cookie first to get the expiration date. Doing that in a stack allocate cookie value would be possible for example but it would result in messier code.
057b6d7
to
fbe6a2d
Compare
Any progress? |
bump |
Hello,
This commit tries to implement a basic CookieJar structure to handle Cookies easily without using external packages.