Fetch the task state only once at the beginning and store it in local variable

[mobiware]

This addresses the following issue:
#80

Looking at Celery code, it turns out that in some cases, self.result.state is being fetched from Celery's backend every time we access it, which means that it can change while we are comparing it to various state constants, as suspected in #80 (comment)

In fact this PR implements just what the above comment ends up suggesting as a potential fix.

[czue]

@OmarWKH are you able to review this one? I think you have the most context.

[OmarWKH]

I suggest also storing other values that we use from self.result, not just self.result.state. They may have the same problem as well.

Looking at celery's source code, anything that uses _get_task_meta may be changed between calls / retrievals if the state is not one of READY_STATES.

https://docs.celeryproject.org/en/stable/_modules/celery/result.html

[mobiware]

@OmarWKH good point. It seems like the only other field that could be subject to a race condition is self.result.info.
Unfortunately, fetching this field and storing it at the beginning of the method does not make us immune against race conditions if the task is not in one of READY_STATES as the task state, and therefore its info field, may change between fetching self.result.state and fetching self.result.info.
The only way I can think of for protecting ourselves against race conditions is by using _get_task_meta() even though it's a private API. I'll push a change proposal using that idea

[OmarWKH]

@mobiware that seems to be the way to go.

This means the client will not know about the actual current state until the next update. Which is fine.

There are 2 places that can still be out of sync:

• self.result.successful()
• self.result.get()

[mobiware]

@OmarWKH I don't think those 2 calls are problematic, because they only happen in the if state in ['SUCCESS', 'FAILURE'] leg, which means that:

• self.result.successful() will use the cached self.result.state because _get_task_meta() caches its result when the state is SUCCESS or FAILURE
• ditto for self.result.get(). Its implementation checks for the existence of a cached task meta, and returns the result from that cache if it exists.

So in both cases we know AsyncResult will use the information that was cached when we called _get_task_meta()

This is far from ideal since it leans on the knowledge of some Celery internals specifics, but this part of Celery source code (the AsyncResult class) doesn't seem like it has changed much over subsequent Celery versions, so I think that the assumptions we're making are pretty safe.

[OmarWKH]

@mobiware you are right.

I'll leave the final approval to @czue.

To summarize the consequences of this PR:
1- It uses celery's private API.
2- The state updates will be insignificantly delayed in some cases.

[czue]

Thanks for working through this and for the clear discussion around the pros/cons and edge cases.

Sounds like you reached the best of a few options - all of which had some downside.

Hope celery doesn't mess with their private API! 🙏