Skip to content

Run integration suite with memcached results backend.#5739

Merged
auvipy merged 1 commit into
masterfrom
memcached-integration-tests
Oct 24, 2019
Merged

Run integration suite with memcached results backend.#5739
auvipy merged 1 commit into
masterfrom
memcached-integration-tests

Conversation

@thedrow

@thedrow thedrow commented Sep 19, 2019

Copy link
Copy Markdown
Contributor

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

Description

The memcache result backend has multiple failures/hangings in the canvas tests.
This is the result of not regularly running the tests with the cache results backend in CI.

This PR introduces the cache backend into the integration test suite matrix.

@thedrow

thedrow commented Sep 19, 2019

Copy link
Copy Markdown
Contributor Author

The failure in test_chain.test_chain_inside_group_receives_arguments is a race condition.
When I sleep inside celery.app.trace.trace_task() or insert a breakpoint the test passes.
However, for some reason when I don't the test fails the same way it does in the build.

EDIT:

I have no idea why sleeping resolves the issue but reverting the fix in #5681 resolves the issue.
Naturally, the relevant integration tests to that PR fail as expected. 😠

EDIT 2:

I still have no idea why sleeping resolved the issue but I found the problem and the correct fix.

@thedrow

thedrow commented Sep 22, 2019

Copy link
Copy Markdown
Contributor Author

The test_group_result_not_has_cache test times out because it can't find the innermost group's result.

t1 = identity.si(1)
t2 = identity.si(2)
gt = group([identity.si(3), identity.si(4)])
ct = chain(identity.si(5), gt)
task = group(t1, t2, ct)

The result should be [1, 2, [3, 4]].
We manage to fetch [1, 2] just fine but the result for [3, 4] is missing for some reason.

EDIT:

It seems like the problem exists for all synchronous results backends:

class SyncBackendMixin(object):
    def iter_native(self, result, timeout=None, interval=0.5, no_ack=True,
                    on_message=None, on_interval=None):
        self._ensure_not_eager()
        results = result.results
        if not results:
            return iter([])
        return self.get_many(
            {r.id for r in results},
            timeout=timeout, interval=interval, no_ack=no_ack,
            on_message=on_message, on_interval=on_interval,
        )
# ...

The last result in results in this case is a GroupResult.

class BaseKeyValueStoreBackend(Backend):
#...
    def get_many(self, task_ids, timeout=None, interval=0.5, no_ack=True,
                 on_message=None, on_interval=None, max_iterations=None,
                 READY_STATES=states.READY_STATES):
        interval = 0.5 if interval is None else interval
        ids = task_ids if isinstance(task_ids, set) else set(task_ids)
        cached_ids = set()
        cache = self._cache
        for task_id in ids:
            try:
                cached = cache[task_id]
            except KeyError:
                pass
            else:
                if cached['status'] in READY_STATES:
                    yield bytes_to_str(task_id), cached
                    cached_ids.add(task_id)

        ids.difference_update(cached_ids)
        iterations = 0
        while ids:
            keys = list(ids)
            r = self._mget_to_results(self.mget([self.get_key_for_task(k)  # There's no task id with this group id
                                                 for k in keys]), keys)
            cache.update(r)
            ids.difference_update({bytes_to_str(v) for v in r})
            for key, value in items(r):
                if on_message is not None:
                    on_message(value)
                yield bytes_to_str(key), value
            if timeout and iterations * interval >= timeout:
                raise TimeoutError('Operation timed out ({0})'.format(timeout))
            if on_interval:
                on_interval()
            time.sleep(interval)  # don't busy loop.
            iterations += 1
            if max_iterations and iterations >= max_iterations:
                break

That's why it hangs forever or times out.

EDIT 2:

The strange thing about this bug is that the same test passes with the DynamoDB results backend which inherits from the KeyValueStoreBackend class and does not override any of those methods.
I'm going to debug it next and see how that behaves to figure out what exactly is wrong with the cache backend.

EDIT 3:

The difference is that DynamoDB does not use join_native because it is not supported on DynamoDB.

class ResultSet(ResultBase):
# ...
    def join(self, timeout=None, propagate=True, interval=0.5,
             callback=None, no_ack=True, on_message=None,
             disable_sync_subtasks=True, on_interval=None):
        """Gather the results of all tasks as a list in order.

        Note:
            This can be an expensive operation for result store
            backends that must resort to polling (e.g., database).

            You should consider using :meth:`join_native` if your backend
            supports it.

        Warning:
            Waiting for tasks within a task may lead to deadlocks.
            Please see :ref:`task-synchronous-subtasks`.

        Arguments:
            timeout (float): The number of seconds to wait for results
                before the operation times out.
            propagate (bool): If any of the tasks raises an exception,
                the exception will be re-raised when this flag is set.
            interval (float): Time to wait (in seconds) before retrying to
                retrieve a result from the set.  Note that this does not have
                any effect when using the amqp result store backend,
                as it does not use polling.
            callback (Callable): Optional callback to be called for every
                result received.  Must have signature ``(task_id, value)``
                No results will be returned by this function if a callback
                is specified.  The order of results is also arbitrary when a
                callback is used.  To get access to the result object for
                a particular id you'll have to generate an index first:
                ``index = {r.id: r for r in gres.results.values()}``
                Or you can create new result objects on the fly:
                ``result = app.AsyncResult(task_id)`` (both will
                take advantage of the backend cache anyway).
            no_ack (bool): Automatic message acknowledgment (Note that if this
                is set to :const:`False` then the messages
                *will not be acknowledged*).
            disable_sync_subtasks (bool): Disable tasks to wait for sub tasks
                this is the default configuration. CAUTION do not enable this
                unless you must.

        Raises:
            celery.exceptions.TimeoutError: if ``timeout`` isn't
                :const:`None` and the operation takes longer than ``timeout``
                seconds.
        """
        if disable_sync_subtasks:
            assert_will_not_block()
        time_start = monotonic()
        remaining = None

        if on_message is not None:
            raise ImproperlyConfigured(
                'Backend does not support on_message callback')

        results = []
        for result in self.results:
            remaining = None
            if timeout:
                remaining = timeout - (monotonic() - time_start)
                if remaining <= 0.0:
                    raise TimeoutError('join operation timed out')
            value = result.get(
                timeout=remaining, propagate=propagate,
                interval=interval, no_ack=no_ack, on_interval=on_interval,
                disable_sync_subtasks=disable_sync_subtasks,
            )  # Collects the GroupResult's here
            if callback:
                callback(result.id, value)
            else:
                results.append(value)
        return results

We need to improve join_native for synchronous backends.
#5638 fixed the same issue for asynchronous backends.
@tothegump Any insights into this problem will be much appreciated.

EDIT 4:

The following patch makes the test pass:

diff --git a/celery/backends/base.py b/celery/backends/base.py
index 468ccc124..e48e0d580 100644
--- a/celery/backends/base.py
+++ b/celery/backends/base.py
@@ -483,7 +483,7 @@ class SyncBackendMixin(object):
         if not results:
             return iter([])
         return self.get_many(
-            {r.id for r in results},
+            {r if isinstance(r, GroupResult) else r.id for r in results},
             timeout=timeout, interval=interval, no_ack=no_ack,
             on_message=on_message, on_interval=on_interval,
         )
@@ -648,6 +648,9 @@ class BaseKeyValueStoreBackend(Backend):
         cached_ids = set()
         cache = self._cache
         for task_id in ids:
+            if isinstance(task_id, GroupResult):
+                continue
+
             try:
                 cached = cache[task_id]
             except KeyError:
@@ -660,15 +663,35 @@ class BaseKeyValueStoreBackend(Backend):
         ids.difference_update(cached_ids)
         iterations = 0
         while ids:
-            keys = list(ids)
-            r = self._mget_to_results(self.mget([self.get_key_for_task(k)
-                                                 for k in keys]), keys)
-            cache.update(r)
-            ids.difference_update({bytes_to_str(v) for v in r})
-            for key, value in items(r):
-                if on_message is not None:
-                    on_message(value)
-                yield bytes_to_str(key), value
+            try:
+                keys = list(ids)
+                consume_until = next(i for i, k in enumerate(keys) if isinstance(k, GroupResult))
+                if consume_until == 0:
+                    g = keys[0]
+                    ids.remove(g)
+                    yield g.id, g.results
+                else:
+                    keys = keys[:consume_until]
+                    r = self._mget_to_results(
+                        self.mget([self.get_key_for_task(k)
+                                   for k in keys]), keys)
+                    cache.update(r)
+                    ids.difference_update({bytes_to_str(v) for v in r})
+                    for key, value in items(r):
+                        if on_message is not None:
+                            on_message(value)
+                        yield bytes_to_str(key), value
+            except StopIteration:
+                keys = list(ids)
+                r = self._mget_to_results(self.mget([self.get_key_for_task(k)
+                                                     for k in keys]), keys)
+                cache.update(r)
+                ids.difference_update({bytes_to_str(v) for v in r})
+                for key, value in items(r):
+                    if on_message is not None:
+                        on_message(value)
+                    yield bytes_to_str(key), value
+
             if timeout and iterations * interval >= timeout:
                 raise TimeoutError('Operation timed out ({0})'.format(timeout))
             if on_interval:

The code in this solution is inelegant at best.
Any suggestions are welcome.

thedrow added a commit that referenced this pull request Sep 23, 2019
…canvas.

PR #5739 uncovered multiple problems with the cache backend.
This PR should resolve one of them.

PR #5638 fixed the same test case for our async results backends that support native join.
However, it did not fix the test case for sync results backends that support native join.
@thedrow

thedrow commented Sep 24, 2019

Copy link
Copy Markdown
Contributor Author

The test_nested_group_chain test fails with:

celery.exceptions.ChordError: GroupResult 114c8570-864c-428a-9821-ae8c060e4a43 no longer exists

I still don't know why.

@tothegump

Copy link
Copy Markdown
Contributor

Sorry for my latency of response.
I'm glad to help.
I would view these related PRs and comments, and make sure I fully understand this PR :)

@thedrow

thedrow commented Oct 5, 2019

Copy link
Copy Markdown
Contributor Author

Sorry for my latency of response.
I'm glad to help.
I would view these related PRs and comments, and make sure I fully understand this PR :)

test_nested_group_chain is still failing and I have no idea why.

@tothegump

Copy link
Copy Markdown
Contributor

Sorry for my latency of response.
I'm glad to help.
I would view these related PRs and comments, and make sure I fully understand this PR :)

test_nested_group_chain is still failing and I have no idea why.

OK, I'll debug it.

@thedrow

thedrow commented Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

Sorry for my latency of response.
I'm glad to help.
I would view these related PRs and comments, and make sure I fully understand this PR :)

test_nested_group_chain is still failing and I have no idea why.

OK, I'll debug it.

Any luck?

@tothegump

Copy link
Copy Markdown
Contributor

Sorry for my latency of response.
I'm glad to help.
I would view these related PRs and comments, and make sure I fully understand this PR :)

test_nested_group_chain is still failing and I have no idea why.

OK, I'll debug it.

Any luck?

Too busy these days, I wish I can fix it this week.

@thedrow
thedrow force-pushed the memcached-integration-tests branch from fd133ff to 6cd07f6 Compare October 24, 2019 10:31
@codecov

codecov Bot commented Oct 24, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5739 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #5739     +/-   ##
=========================================
- Coverage   83.39%   83.29%   -0.1%     
=========================================
  Files         144      144             
  Lines       16702    16775     +73     
  Branches     2080     2101     +21     
=========================================
+ Hits        13928    13973     +45     
- Misses       2560     2586     +26     
- Partials      214      216      +2
Impacted Files Coverage Δ
celery/contrib/migrate.py 97.9% <0%> (-0.53%) ⬇️
celery/app/amqp.py 94.53% <0%> (-0.33%) ⬇️
celery/backends/base.py 96.09% <0%> (-0.05%) ⬇️
celery/canvas.py 93.07% <0%> (-0.02%) ⬇️
celery/result.py 63.18% <0%> (+0.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1076d83...6cd07f6. Read the comment docs.

@thedrow

thedrow commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

I'd like to merge this as the other fixes are relevant for other backends as well.
I'll open an issue for the last failure.

@auvipy

auvipy commented Oct 24, 2019

Copy link
Copy Markdown
Member

wait let me check before the merge.

@auvipy
auvipy merged commit 59c7872 into master Oct 24, 2019
@thedrow
thedrow deleted the memcached-integration-tests branch October 24, 2019 11:11
thedrow added a commit that referenced this pull request Oct 24, 2019
…canvas.

PR #5739 uncovered multiple problems with the cache backend.
This PR should resolve one of them.

PR #5638 fixed the same test case for our async results backends that support native join.
However, it did not fix the test case for sync results backends that support native join.
thedrow added a commit that referenced this pull request Oct 24, 2019
…canvas. (#5744)

PR #5739 uncovered multiple problems with the cache backend.
This PR should resolve one of them.

PR #5638 fixed the same test case for our async results backends that support native join.
However, it did not fix the test case for sync results backends that support native join.