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.

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

net/http: Cookiejar interface functions Cookies and SetCookies do not return error #54639

Open
hi117 opened this issue Aug 24, 2022 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@hi117
Copy link

hi117 commented Aug 24, 2022

What version of Go are you using (go version)?

$ go version
go version go1.19 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
N/A

What did you do?

Attempted to use cookie jar to store cookies using a method that might produce errors.

What did you expect to see?

A way to propagate errors to the caller without calling Panic() or ignoring

What did you see instead?

No way to propagate errors to the caller besides a full Panic() or ignoring

@panjf2000
Copy link
Member

panjf2000 commented Aug 24, 2022

Could you provide some sample code and stack trace output after panics? It would help a lot with this issue, thanks!

@hi117
Copy link
Author

hi117 commented Aug 24, 2022

Sorry, I was a bit unclear, I'm actually trying to implement the interface defined in net/http.

@hi117 hi117 changed the title net/http/cookiejar: Cookies and SetCookies do not return error net/http: Cookiejar Interface Functions Cookies and SetCookies do not return error Aug 24, 2022
@hi117 hi117 changed the title net/http: Cookiejar Interface Functions Cookies and SetCookies do not return error net/http: Cookiejar interface functions Cookies and SetCookies do not return error Aug 24, 2022
@hi117
Copy link
Author

hi117 commented Aug 24, 2022

I also realized that it doesn't accept context either, which means if your cookie jar makes calls to external services (ie redis, a sql database), these become untrackable in tracing and uncancelable if they take a long time. Ideally the interface would be changed to be more like this:

type CookieJar interface {
	// SetCookies handles the receipt of the cookies in a reply for the
	// given URL.  It may or may not choose to save the cookies, depending
	// on the jar's policy and implementation.
	SetCookies(c context.Context, u *url.URL, cookies []*Cookie) error

	// Cookies returns the cookies to send in a request for the given URL.
	// It is up to the implementation to honor the standard cookie use
	// restrictions such as in RFC 6265.
	Cookies(c context.Context, u *url.URL) ([]*Cookie, error)
}

Hopefully this is more clear. I also see that this was an issue during intial development since if the official implementation of cookiejar encounters an error it just silently returns no cookies: https://github.com/golang/go/blob/master/src/net/http/cookiejar/jar.go#L167

This is probably not ideal since if you're relying on the cookiejar to handle auth to an API, you would probably want the request to be canceled with an error that the caller can handle rather than continuing the request with no cookies.

@dr2chase
Copy link
Contributor

dr2chase commented Aug 24, 2022

This seems like a request for a change to the standard library, and might need a proposal of some sort. We can't break existing code by changing interfaces that they might call or implement.
@neild @rsc

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2022
@neild
Copy link
Contributor

neild commented Aug 24, 2022

As @dr2chase says, we can't change the CookieJar interface.

Working within the constraints of the current interface, I can imagine having a paired RoundTripper wrapper and CookieJar implementation, where RoundTrip sets the jar's context before proceeding and checks its error status upon completion. This would be pretty clumsy, but probably could be made to work.

An updated CookieJar API that accepts a context.Context and returns an error doesn't seem unreasonable, but it'd need some thought into what the exact API should be and how to integrate it into the existing net/http APIs without backwards compatibility issues, as well as a proposal.

@hi117
Copy link
Author

hi117 commented Aug 25, 2022

🤔 Ignoring the existing CookieJar interface and just implementing a RoundTripper does seem like a good idea within the confines of the existing interface.

As for updating the interface so that its actually consumable rather than just being ignored, my thought was to make a CookieJarV2 interface and field in Client that if specified takes precedence over regular CookieJar. Behavior would be that if a "non nil" CookieJarV2 were given, to use that always. If a "nil" value is given to the Client, then to fall back to old CookieJar and the default of both being nil means cookies are only sent if they are explicitly set on the Request.

This should give full backward compatibility. Docs would just mark old CookieJar as deprecated and sometime during golang 2.0 it can be removed entirely. Its kinda ugly IMO and maybe confusing, but the behavior of unaware code shouldn't change.

@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants