Support Context on API requests

[brandur]

Supports the Context struct [1] that came in with Go 1.7 on Stripe API
requests. This can be used to carry deadlines, cancelation signals, and
other request-scoped values across API boundaries and between processes.

Fixes #357.

r? @remi-stripe

[1] https://golang.org/pkg/context/

[remi-stripe]

@brandur-stripe That looks mostly good to me. This is a bit beyond what I know of Go though and I am not sure I would see edge-case if you missed something.

Is it something we could ask the community to help review? Especially the ones in the original issue reported?

[brandur-stripe]

Opportunistic fix here. "Checkin"'s been stripped everywhere else.

[brandur-stripe]

Is it something we could ask the community to help review? Especially the ones in the original issue reported?

Good point. I asked in the other thread; hopefully we can get some volunteers!

[derekperkins]

The test here is pretty simple, so it might be nice to do a context.WithTimeout to prepare for deadlines being reached. This also makes the idempotency key even more important, since the context isn't propagating to the server. I might even go so far as to require idempotency if you are supplying a context, even if just to prevent users from shooting themselves in the foot.

Overall, this is a very elegant way to add context support while remaining BC.

[brandur-stripe]

Thanks @derekperkins!

The test here is pretty simple, so it might be nice to do a context.WithTimeout to prepare for deadlines being reached.

I added a new test in d6b3d79. I avoided WithTimeout because I don't want to introduce any weird race conditions, but did something similar by initializing a context WithCancel and then canceling the context before even trying to execute the request. When we try to execute it, it errors immediately with a message that we can recognize as having originated from the canceled context.

This also makes the idempotency key even more important, since the context isn't propagating to the server. I might even go so far as to require idempotency if you are supplying a context, even if just to prevent users from shooting themselves in the foot.

Can you run this by me one more time? I'm not sure I follow. Reusing a context isn't going to persist headers or anything is it?

[derekperkins]

I avoided WithTimeout because I don't want to introduce any weird race conditions, but did something similar by initializing a context WithCancel and then canceling the context before even trying to execute the request.

I wasn't sure if you had a mock of some kind that would allow you to add a pre-defined delay to get repeatable results.

Can you run this by me one more time? I'm not sure I follow. Reusing a context isn't going to persist headers or anything is it?

No, context won't persist anything that you don't want it to. Before context, there was always the possibility of a network error or something that would cause acknowledgment to fail, hence the idempotency key. With context, there is generally going to be a timeout, increasing the likelihood that the user thinks a transaction failed when it actually went through on the server. The suggestion to require the two together was mostly tongue in cheek, but I wouldn't ever use it without the idempotency key.

[klauspost]

I know we aren't dealing with newbie programmers, but maybe explicitly stating something like:

// A cancelled or timed out context does not provide any guarantee 
// whether the operation was or was not completed on the backend.
// You must still perform appropriate rollback or retry with the same idempotency key.

[derekperkins]

👍

[derekperkins]

provide any type guarantee

should either read provide any guarantee or provide any type of guarantee

[brandur-stripe]

I like it. I tweaked your comment a bit and committed in 1127516.

[brandur-stripe]

No, context won't persist anything that you don't want it to. Before context, there was always the possibility of a network error or something that would cause acknowledgment to fail, hence the idempotency key. With context, there is generally going to be a timeout, increasing the likelihood that the user thinks a transaction failed when it actually went through on the server. The suggestion to require the two together was mostly tongue in cheek, but I wouldn't ever use it without the idempotency key.

@derekperkins Ah I see now. Thank you!

I actually really like the idea of checking for an idempotency key as well before Context is allowed to be used and even started writing the code for it, but I've decided that for now to allow this possible footgun and allow users to control their own destiny in this respect.

You're right in that an idempotency key is almost certainly nearly always desirable, but there are a few cases where you can legitimately get away without one like on any type of non-modification request (e.g. say you're retrieving a list or something).

I still want to rework how idempotency works in this library a bit (it should probably send an idempotency key by default if one was not specified for example), and I might revisit the decision at that time.