Skip to content
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.

Sign up for GitHub

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

Jump to bottom

proposal: net/http/cookiejar: add Jar.Clear #70258

Open
AidanWelch opened this issue Nov 8, 2024 · 5 comments · May be fixed by #70259
Open

proposal: net/http/cookiejar: add Jar.Clear #70258

AidanWelch opened this issue Nov 8, 2024 · 5 comments · May be fixed by #70259
Labels
Proposal
Milestone
Proposal

Comments

@AidanWelch
Copy link

AidanWelch commented Nov 8, 2024 •
edited
Loading

Proposal Details

Currently, since a jar's entries are not exposed to clear all cookies in a client's Jar in a thread-safe manner would require(from my understanding):

  • Wrapping all calls to the client in a read lock, then write locking as the client's Jar is replaced with a new empty jar.
  • Keeping track of all URLs accessed in a thread-safe manner, getting the cookies for each of them, and overwriting each of those cookies with expired ones,
  • Creating a Jar wrapping type storing a pointer to the jar that has an additional lock on each of the interface functions, and also a third Clear function that sets the pointer to the new jar. This is probably the most reasonable option.

Neither of those options are especially convenient, so I propose cookiejar adds a Clear() method that I will open a PR for. I am not proposing this is added to net/http's CookieJar as that would be a breaking change.
@gopherbot gopherbot added this to the Proposal milestone Nov 8, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Nov 8, 2024
AidanWelch added a commit to AidanWelch/go that referenced this issue Nov 8, 2024
@AidanWelch
net/http/cookiejar: implement concurrent-safe Clear method
32a6064 
Currently clearing all stored cookies puts a needless burden on package consumers, especially when used within http/Client where the calls to the cookiejar are not directly done by the consumer.  This change just directly implements a safe clear of all entries.

Fixes golang#70258
@AidanWelch AidanWelch linked a pull request Nov 8, 2024 that will close this issue
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/625898 mentions this issue: net/http/cookiejar: implement concurrent-safe Clear method

@seankhliao
Copy link
Member

What's the use case of clearing everything? and is this the appropriate thing to do if you have in flight requests?
Shouldn't clearing only be done when you know requests are all done, at which point you can just set Client.Jar to a new cookie jar?

@seankhliao seankhliao changed the title proposal: net/http/cookiejar: Implement a concurrency-safe method to clear the cookiejar proposal: net/http/cookiejar: add Clear Nov 8, 2024
@seankhliao seankhliao changed the title proposal: net/http/cookiejar: add Clear proposal: net/http/cookiejar: add Jar.Clear Nov 8, 2024
@AidanWelch
Copy link
Author

AidanWelch commented Nov 9, 2024 •
edited
Loading

My use case for it is making concurrent requests with an authenticated client until X time, at which point refreshing the auth cookies/tracking for the client. (And in this case inflight requests largely wouldn't matter, the cookies were either sent or weren't)

@seankhliao
Copy link
Member

that seems quite niche, and easily solvable by wrapping the stdlib implementation

@AidanWelch
Copy link
Author

That's fair, my reason for proposal was just how trivial it is to implement within the cookie jar, while requiring additional mutexes and state tracking outside it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

Successfully merging a pull request may close this issue.

net/http/cookiejar: implement concurrent-safe Clear method AidanWelch/go
3 participants
@gopherbot @seankhliao @AidanWelch