Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Potential payday race condition #2022

Closed
coderanger opened this issue Feb 12, 2014 · 7 comments
Closed

Potential payday race condition #2022

coderanger opened this issue Feb 12, 2014 · 7 comments

Comments

@coderanger
Copy link
Contributor

If a payday is interrupted and then resumed later, but in the gap someone opted-in to gittip it will produce inconsistencies. If I'm reading this code right, https://github.com/gittip/www.gittip.com/blob/master/gittip/models/participant.py#L661 should be checking if the claimed_time <= for_payday (read: they claimed their account before this payday started).

@coderanger
Copy link
Contributor Author

I'm also somewhat confused by genparticipants being called three times in the run() method, but I'm not sure there's an actual race condition there or just inefficiency.

@chadwhitacre
Copy link
Contributor

@zwn Can you say more about how this relates to #1428? Or otherwise review this ticket? :-)

@zbynekwinkler
Copy link
Contributor

Sorry, it does not relate. And @coderanger is right, genparticipants is not idempotent and it should be if we hope to resume the payday after an error.

@chadwhitacre
Copy link
Contributor

My recollection is that I decided that genparticipants not being idempotent wasn't a big deal. I recall noticing that a payday rerun (which is not theoretical, it does happen occasionally) would be subtly different than the first run, but in a way that was acceptable. I guess I didn't document that in the comments?

@zbynekwinkler
Copy link
Contributor

I wasn't super scared reading the report either. But still, it would be better to have it be idempotent.

@chadwhitacre
Copy link
Contributor

Cool, I've marked this ★★☆.

@Changaco
Copy link
Contributor

Payday has been rewritten (#2508).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants