even if it is cached no_ack should affect to the get function

[nistick21]

Note: Before submitting this pull request, please review our contributing
guidelines.

Description

I'm beginer of celery. I'm trying to ampq as broker and backend.

in tutorial, result is not removed by get() function

for example,

case1

result = add.delay(4, 4)
result.ready()
result.get()

case2

result = add.delay(4, 4)
result.get()

when I was trying to case 1, not removed from result queue.
but case 2 does.

so I add 1 line in get function.

I'm not good at programming, If you have a another solution. plz tell me.

thanks.

[codecov]

Codecov Report

Base: 89.77% // Head: 89.77% // No change to project coverage 👍

Coverage data is based on head (3c39739) compared to base (e0b0af6).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7852   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files         128      128           
  Lines       15790    15790           
  Branches     2111     2111           
=======================================
  Hits        14176    14176           
  Misses       1386     1386           
  Partials      228      228           

Flag	Coverage Δ
unittests	89.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[auvipy]

can you please add a unit test to verify this change?

[nistick21]

I have no idea about unit test, can you explain how to do this?

[nistick21]

To explain a little more in detail, if it becomes a cache through the ready function, no_ack is False and the consumer operates. After that, through the get function, the consumer should operate with no_ack set to True, but the cache value is returned and the function is terminated without operating the consumer with no_ack set to True.

I don't know how make exactly test code. I'm not sure how to write code that correct tests this phenomenon.

[auvipy]

you can check this file to have an idea about related tests https://github.com/celery/celery/blob/master/t/unit/tasks/test_result.py you can try to figure out and learn something new. but if you can not at all then we will see

[auvipy]

@Nusnus feel free to give this a tryand let me know what do you think about this change

[Nusnus]

From what I can see, the issue makes sense, but I'm not sure if we want this exact solution.
ready() indeed creates a cache which explains why get(no_ack=True) would return from the cache.
That being said, changing get() implementation to skip the cache on no_ack==True may lead to confusion, as this is a specific-case issue and you don't want to tie the caching to the task acknowledgment just for that.
Second, this may break the API for cases where a cache exists, and you call get() (with default no_ack==True), when the expected behavior would be to get from the cache, but this change will break this (== cache is valid but not used due to acknowledgment configuration).
So the problem is that we want to separate the caching and the acknowledgment and this PR creates a dependency that breaks the API as well which is why I don't think it qualifies with the current implementation @auvipy

@nistick21 In other words, your fix applies well to your case but will cause other people to have a different behavior than expected without an explicit change, which is prone to issues that are hard to find and fix, especially in production systems. So we have to be careful about what we change, even if all of the unit/integration tests of the CI are passing.

Lastly, if I assume correctly and the end results are to be able to change the state of the task and get the result without invoking the cache, we need an explicit and clear way of implementing this. An example off the top of my head may be adding a (default False) argument to get() to ignore cache, for example:

def get(self, timeout=None, propagate=True, interval=0.5,
            no_ack=True, follow_parents=True, callback=None, on_message=None,
            on_interval=None, disable_sync_subtasks=True,
            EXCEPTION_STATES=states.EXCEPTION_STATES,
            PROPAGATE_STATES=states.PROPAGATE_STATES,
            ignore_cache=False):
...
if not ignore_cache and self._cache:
    if propagate:
        self.maybe_throw(callback=callback)
    return self.result

Then you can have your example like this:

result = add.delay(4, 4)
result.ready()
result.get(ignore_cache=True)

result = add.delay(4, 4)
result.get()

This will make the code much more readable, as the result.get(ignore_cache=True) will tell 100% what is happening and will remove confusion. In addition, it will not create a dependency between task acknowledgment and result caching.

If this makes sense @nistick21 and you wish to take this further, LMK and I'll try to push you in the right direction. For now here are my two cents :)

P.S
This "proposed solution" is just to convey my point - there might be better implementations, but it requires more investigation.