I am seeing some failures in travis and some buildbots:

0:02:24 load avg: 3.30 [147/420/1] test_slice crashed (Exit code -6) -- running: test_multiprocessing_spawn (32 sec 523 ms), test_asyncio (45 sec 433 ms), test_multiprocessing_forkserver (47 sec 869 ms)
Modules/gcmodule.c:110: gc_decref: Assertion "gc_get_refs(g) > 0" failed: refcount is too small
Enable tracemalloc to get the memory block allocation traceback
object  : <test.test_slice.SliceTest.test_cmp.<locals>.BadCmp object at 0x2ab2051faef0>
type    : BadCmp
refcount: 1
address : 0x2ab2051faef0
Fatal Python error: _PyObject_AssertFailed
Current thread 0x00002ab1fe0519c0 (most recent call first):
  File "/home/travis/build/python/cpython/Lib/test/test_slice.py", line 107 in <lambda>
  File "/home/travis/build/python/cpython/Lib/unittest/case.py", line 197 in handle
  File "/home/travis/build/python/cpython/Lib/unittest/case.py", line 782 in assertRaises
  File "/home/travis/build/python/cpython/Lib/test/test_slice.py", line 107 in test_cmp
  File "/home/travis/build/python/cpython/Lib/unittest/case.py", line 642 in run
  File "/home/travis/build/python/cpython/Lib/unittest/case.py", line 702 in __call__
  File "/home/travis/build/python/cpython/Lib/unittest/suite.py", line 122 in run
  File "/home/travis/build/python/cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/home/travis/build/python/cpython/Lib/unittest/suite.py", line 122 in run
  File "/home/travis/build/python/cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/home/travis/build/python/cpython/Lib/unittest/suite.py", line 122 in run
  File "/home/travis/build/python/cpython/Lib/unittest/runner.py", line 176 in run
  File "/home/travis/build/python/cpython/Lib/test/support/__init__.py", line 1935 in _run_suite
  File "/home/travis/build/python/cpython/Lib/test/support/__init__.py", line 2031 in run_unittest
  File "/home/travis/build/python/cpython/Lib/test/libregrtest/runtest.py", line 178 in test_runner
  File "/home/travis/build/python/cpython/Lib/test/libregrtest/runtest.py", line 182 in runtest_inner
  File "/home/travis/build/python/cpython/Lib/test/libregrtest/runtest.py", line 127 in runtest
  File "/home/travis/build/python/cpython/Lib/test/libregrtest/runtest_mp.py", line 68 in run_tests_worker
  File "/home/travis/build/python/cpython/Lib/test/libregrtest/main.py", line 600 in _main
  File "/home/travis/build/python/cpython/Lib/test/libregrtest/main.py", line 586 in main
  File "/home/travis/build/python/cpython/Lib/test/libregrtest/main.py", line 640 in main
  File "/home/travis/build/python/cpython/Lib/test/regrtest.py", line 46 in _main
  File "/home/travis/build/python/cpython/Lib/test/regrtest.py", line 50 in <module>
  File "/home/travis/build/python/cpython/Lib/runpy.py", line 85 in _run_code

Usually, this happens with test_slice but when the test runner runs test_slice in isolation, it succeeds.

I am afraid that this will be a weird combination of tests.

I too saw this a week back but couldn't reproduce it. There is one another test that fails like this and passes on verbose mode issue35809 . Travis doesn't report tests that fail in the complete run and pass during verbose run like buildbots do.

bpo-35977 has been marked as a duplicate, copy of the first message:

s390x Debian 3.x:
https://buildbot.python.org/all/#/builders/13/builds/2344

0:06:26 load avg: 0.92 [291/420/1] test_slice crashed (Exit code -6)
Modules/gcmodule.c:110: gc_decref: Assertion "gc_get_refs(g) > 0" failed: refcount is too small
Enable tracemalloc to get the memory block allocation traceback

object  : <test.test_slice.SliceTest.test_cmp.<locals>.BadCmp object at 0x3ff93967e90>
type    : BadCmp
refcount: 1
address : 0x3ff93967e90
Fatal Python error: _PyObject_AssertFailed

Current thread 0x000003ff95272700 (most recent call first):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/test/test_slice.py", line 107 in <lambda>
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/unittest/case.py", line 197 in handle
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/unittest/case.py", line 782 in assertRaises
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/test/test_slice.py", line 107 in test_cmp
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/unittest/case.py", line 642 in run
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/unittest/case.py", line 702 in __call__
  ...

> I am seeing some failures in travis and some buildbots:

Do you have an url to an example of crash on Travis?

Which buildbots are affected? I saw the crash on s390x Debian 3.x.

I'm unable to reproduce the issue on my Fedora 29 (x86-64) laptop. I used two terminals:

* ./python -m test -F -j0 test_slice test_slice test_slice test_slice test_slice test_slice test_slice test_slice
* ./python -m test -j0 -u all,-gui -r # stress the system to randomize timings in test_slice

Extract of the crashing test:

    def test_cmp(self):
        ...

        class Exc(Exception):
            pass

        class BadCmp(object):
            def __eq__(self, other):
                raise Exc

        s1 = slice(BadCmp())
        s2 = slice(BadCmp())
        self.assertEqual(s1, s1)
        self.assertRaises(Exc, lambda: s1 == s2)   # <==== CRASH HERE

        ...

The latest change in test_slice.py has been made in 2016: 3 years ago.

Latest changes in Objects/sliceobject.c were refactoring related to header files (november 2018). The latest "significant" change was done in April 2018: "bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS".

I bet more on a recent change which is not directly related to slices... like the implementation of the PEP 572? It would help to know when the crash started to occur exactly (date + time).

Lukasz: I'm not sure what is happening here, but such random bugs are always scrary. I raise the priority to "release blocker" until we know more about the issue.

On s390x Debian 3.x, it seems like the bug started to show up near:
https://buildbot.python.org/all/#/builders/13/builds/2328

This build contains 2 changes, one doc change and this namedtuple change which mentions reference count:

https://github.com/python/cpython/commit/64360ada0f6123a051e9dc6cd04f030ec1322e46

"Complete and neaten-up namedtuple's replacement of builtin function lookups with derefs (GH-11794)"

Ok, but I don't see the relationship between namedtupled and this specific test!?

> Do you have an url to an example of crash on Travis?

Travis crash that occurred few days back : https://travis-ci.org/python/cpython/jobs/491701299 . I first noticed it on an IDLE lib PR where this failure was unrelated but I can remember occurring at least few weeks back. I will try get the older one. Sorry I forgot to file an issue during that time.

Ah! I found a way to reproduce the crash:

diff --git a/Lib/test/test_slice.py b/Lib/test/test_slice.py
index 4ae4142c60..b18f8f1c8c 100644
--- a/Lib/test/test_slice.py
+++ b/Lib/test/test_slice.py
@@ -1,4 +1,8 @@
 # tests for slice objects; in particular the indices method.
+import gc, random
+n = random.randint(1, 700)
+print("set_threshold(%s)" % n)
+gc.set_threshold(n)
 
 import itertools
 import operator


$ ./python -m test -F -j0 test_slice test_slice test_slice test_slice test_slice test_slice test_slice test_slice


I went up to January 1st, 2018 (commit e8ed96550c6aa9a1e39c36e67e892994e25e2c41): the test already crashed with my patch, so it's not a regression. I reset the priority.

See also this idea of changing gc.set_threshold() when using -X dev:
https://mail.python.org/pipermail/python-dev/2018-June/153857.html

It has been crashing for almost all the last 5 PRs merged to master : 

https://travis-ci.org/python/cpython/jobs/492241988 - Feb 12, 21:51
https://travis-ci.org/python/cpython/jobs/492123432 - Feb 12, 17:33
https://travis-ci.org/python/cpython/jobs/491917875 - Feb 12, 06:20
https://travis-ci.org/python/cpython/jobs/491898690 - Feb 12, 05:15
https://travis-ci.org/python/cpython/jobs/491798997 - Feb 12, 01:19

It's easy to overlook this since PR passes on verbose mode . There is another test these days that fails very frequently but passes on verbose mode : issue35809 . It's little hard to track their first failure since Travis provides no search capability for logs.

I removed Lukasz from the nosy list since it's not a regression. No need to spam our 3.8 release manager ;-)

Extract of slice_richcompare():

    t1 = PyTuple_New(3);
    if (t1 == NULL)
        return NULL;
    t2 = PyTuple_New(3);
    if (t2 == NULL) {
        Py_DECREF(t1);
        return NULL;
    }

    PyTuple_SET_ITEM(t1, 0, ((PySliceObject *)v)->start);
    PyTuple_SET_ITEM(t1, 1, ((PySliceObject *)v)->stop);
    PyTuple_SET_ITEM(t1, 2, ((PySliceObject *)v)->step);
    PyTuple_SET_ITEM(t2, 0, ((PySliceObject *)w)->start);
    PyTuple_SET_ITEM(t2, 1, ((PySliceObject *)w)->stop);
    PyTuple_SET_ITEM(t2, 2, ((PySliceObject *)w)->step);

    res = PyObject_RichCompare(t1, t2, op);

    PyTuple_SET_ITEM(t1, 0, NULL);
    PyTuple_SET_ITEM(t1, 1, NULL);
    PyTuple_SET_ITEM(t1, 2, NULL);
    PyTuple_SET_ITEM(t2, 0, NULL);
    PyTuple_SET_ITEM(t2, 1, NULL);
    PyTuple_SET_ITEM(t2, 2, NULL);

    Py_DECREF(t1);
    Py_DECREF(t2);

    return res;

t1 and t2 tuples are tracked by the GC, but t1 and t2 items are *borrowed* references.

gc.collect() calls subtract_refs() which decreases the reference counter of all objects tracked by the GC... but v, w, t1 and t2 contain the same references which causes the assertion error.

The code is quite old:

commit 47b9ff6ba11fab4c90556357c437cb4feec1e853
Author: Guido van Rossum <guido@python.org>
Date:   Thu Aug 24 00:41:19 2006 +0000

    Restructure comparison dramatically.  There is no longer a default
    *ordering* between objects; there is only a default equality test
    (defined by an object being equal to itself only).  Read the comment
    in object.c.  The current implementation never uses a three-way
    comparison to compute a rich comparison, but it does use a rich
    comparison to compute a three-way comparison.  I'm not quite done
    ripping out all the calls to PyObject_Compare/Cmp, or replacing
    tp_compare implementations with tp_richcompare implementations;
    but much of that has happened (to make most unit tests pass).
    (...)

+static PyObject *
+slice_richcompare(PyObject *v, PyObject *w, int op)
...
+       t1 = PyTuple_New(3);
+       t2 = PyTuple_New(3);
+       if (t1 == NULL || t2 == NULL)
+               return NULL;
+
+       PyTuple_SET_ITEM(t1, 0, ((PySliceObject *)v)->start);
+       PyTuple_SET_ITEM(t1, 1, ((PySliceObject *)v)->stop);
+       PyTuple_SET_ITEM(t1, 2, ((PySliceObject *)v)->step);
+       PyTuple_SET_ITEM(t2, 0, ((PySliceObject *)w)->start);
+       PyTuple_SET_ITEM(t2, 1, ((PySliceObject *)w)->stop);
+       PyTuple_SET_ITEM(t2, 2, ((PySliceObject *)w)->step);
...

I see two options:

* Ensure that t1 and t2 temporary objects are not tracked by the GC
* Modify slice_richcompare() to use strong references, rather than *borrowed* references

I wrote PR 11828 which implements the first solution. The second solution has a minor impact on performance.

It's impressive *and* scary that such 13 years old bug only show up today...

I modified the GC threshold in the site module with this patch:

diff --git a/Lib/site.py b/Lib/site.py
index ad1146332b..68cdf105b1 100644
--- a/Lib/site.py
+++ b/Lib/site.py
@@ -557,6 +557,10 @@ def main():
     This function is called automatically when this module is imported,
     unless the python interpreter was started with the -S flag.
     """
+    import gc, random
+    n = random.randint(5, 100)
+    gc.set_threshold(n)
+
     global ENABLE_USER_SITE
 
     orig_path = sys.path[:]


Running the test suite with this patch doesn't catch the bug :-(

$ ./python -m test -j0 -r -u all,-gui

Victor found the same bug I found while I was composing this, posting only to incorporate proposed solution:

I *think* I have a cause for this, but someone else with greater understanding of the cycle collector should check me if the suggested fix has non-trivial performance implications (I suspect the answer is no, performance is unaffected).

slice_richcompare borrows its behavior from tuple by creating a temporary tuple for each slice, the delegating to the tuple comparison ( https://github.com/python/cpython/blob/master/Objects/sliceobject.c#L591 ).

Problem is, it uses borrowed references when creating said tuples, not owned references. Because test_slice's BadCmp.__eq__ is implemented in Python, the comparison can be interrupted by cycle collection during the __eq__ call. When then happens, there are precisely two references to the BadCmp object:

1. In the slice (owned)
2. In the temporary tuple (borrowed)

When a cycle collection occurs during the comparison, and subtract_refs ( https://github.com/python/cpython/blob/master/Modules/gcmodule.c#L398 ) is called, the BadCmp object in question is visited via both the slice and the tuple, and since it has no non-container objects referencing it, it ends up with the initial reference count of 1 attempting to drop to -1, and the assertion is violated. While the code of gcmodule.c appears to have been refactored since 3.7 so the assert occurs in a different function, with a slightly different message, it would break the same way in both 3.7 and master, and whether or not it triggers the bug, the broken behavior of slice_richcompare hasn't changed for a *long* time. 

Underlying problem would seem to be slice's richcompare believing it's okay to make a tuple from borrowed references, then make a call on it that can trigger calls into Python level code (and therefore into the cycle collector); everything else is behaving correctly here. I'm guessing the only reason it's not seen in the wild is that slices based on Python defined types are almost never compared at all, let alone compared on debug builds that would be checking the assert and with an accelerated cycle collection cycle that would make a hit likely.

Solution would be to stop trying to microoptimize slice_richcompare to avoid reference count manipulation and just build a proper tuple. It would even simplify the code since we could just use PyTuple_Pack, reducing custom code by replacing:

    t1 = PyTuple_New(3);
    if (t1 == NULL)
        return NULL;
    t2 = PyTuple_New(3);
    if (t2 == NULL) {
        Py_DECREF(t1);
        return NULL;
    }

    PyTuple_SET_ITEM(t1, 0, ((PySliceObject *)v)->start);
    PyTuple_SET_ITEM(t1, 1, ((PySliceObject *)v)->stop);
    PyTuple_SET_ITEM(t1, 2, ((PySliceObject *)v)->step);
    PyTuple_SET_ITEM(t2, 0, ((PySliceObject *)w)->start);
    PyTuple_SET_ITEM(t2, 1, ((PySliceObject *)w)->stop);
    PyTuple_SET_ITEM(t2, 2, ((PySliceObject *)w)->step);

with:

    t1 = PyTuple_Pack(3, ((PySliceObject *)v)->start, ((PySliceObject *)v)->stop, ((PySliceObject *)v)->step);
    if (t1 == NULL)
        return NULL;
    t2 = PyTuple_Pack(3, ((PySliceObject *)w)->start, ((PySliceObject *)w)->stop, ((PySliceObject *)w)->step);
    if (t2 == NULL) {
        Py_DECREF(t1);
        return NULL;
    }

and makes cleanup simpler, since you can just delete:

    PyTuple_SET_ITEM(t1, 0, NULL);
    PyTuple_SET_ITEM(t1, 1, NULL);
    PyTuple_SET_ITEM(t1, 2, NULL);
    PyTuple_SET_ITEM(t2, 0, NULL);
    PyTuple_SET_ITEM(t2, 1, NULL);
    PyTuple_SET_ITEM(t2, 2, NULL);

and let the DECREFs for t1/t2 do their work normally.

If for some reason the reference count manipulation is unacceptable, this *could* switch between two behaviors depending on whether or not start/stop/step are of known types (e.g. if all are NoneType/int, this could use the borrowed refs code path safely) where a call back into Python level code is impossible; given that slices are usually made of None and/or ints, this would remove most of the cost for the common case, at the expense of more complicated code. Wouldn't help numpy types though, and I suspect the cost of pre-checking the types for all six values involved would eliminate most of the savings.

Sorry for not submitting a proper PR; the work machine I use during the day is not suitable for development (doesn't even have Python installed).

> It's impressive *and* scary that such 13 years old
> bug only show up today...

Then again, there's probably no other code in the world that compares slice objects ;-)

Ah, I see Victor posted an alternative PR that avoids the reference counting overhead by explicitly removing the temporary tuples from GC tracking. I'm mildly worried by that approach, only because the only documented use case for PyObject_GC_UnTrack is in tp_dealloc (that said, the code explicitly allows it to be called twice due to the Py_TRASHCAN mechanism, so it's probably safe so long as the GC design never changes dramatically). If slice comparison really is performance sensitive enough to justify this, so be it, but I'd personally prefer to reduce the custom code involved in a rarely used code path (we're not even caching constant slices yet, so no comparisons are likely to occur for 99.99% of slices, right?).

I wrote PR 11830: the other solution that I propose, use strong references rather than borrowed references.

Josh, I'd say the speed of this code doesn't matter one whit to anything.  Like you, I'd _prefer_ that the issue be fixed by building "real tuples" that own their own references, which would also (as you showed) allow for briefer, simpler, clearer code.  But I'm OK with playing obscure GC tricks too.

> Then again, there's probably no other code in the world that compares slice objects ;-)

Well, that's a good explanation :-) So maybe there is no need to keep the micro-optimization and PR 11830 would be safer (use strong references), rather than PR 11828 (untrack the 2 internal tuples).

Oh, Tim Peters succeded somehow to remove Josh Rosenberg from the nosy list: I add him again ;-)

Thanks Josh for your analysis, it seems like we agree :-) I wrote PR 11830 to use strong references, as you proposed.

> Oh, Tim Peters succeded somehow to
> remove Josh Rosenberg from the nosy list: I add him again ;-)

Certainly wasn't my intent!  That happens too often on the tracker.  Thanks for noticing! :-(

+1 on PR 11830 from me. Can always revisit if #11107 is ever implemented and it turns out that the reference count manipulation means startup is too slow due to all the slice interning triggered comparisons (unlikely, but theoretically possible I guess).

New changeset dcb68f47f74b0cc8a1896d4a4c5a6b83c0bbeeae by Victor Stinner in branch 'master':
bpo-35961: Fix a crash in slice_richcompare() (GH-11830)
https://github.com/python/cpython/commit/dcb68f47f74b0cc8a1896d4a4c5a6b83c0bbeeae

Python 2.7 is not affected: it doesn't have the slice_richcompare() function, but the old slice_compare() implementation.

Python 3.6 is affected but doesn't accept bugfixes anymore: https://devguide.python.org/#status-of-python-branches This bug is 13 years old, I don't think that it's really useful to fix it. It mostly impact test_slice and only test_slice.

> This bug is 13 years old, I don't think that it's really useful to fix it. It mostly impact test_slice and only test_slice.

To be clear, I compiled Python 3.6 in release mode and applied test_slice patch of msg335333: no crash after 1500 runs. The bug is an assertion error, but assertions are only compiled in debug mode.

New changeset a78251e2d6de37f46c230be924032a87df730761 by Miss Islington (bot) in branch '3.7':
bpo-35961: Fix a crash in slice_richcompare() (GH-11830)
https://github.com/python/cpython/commit/a78251e2d6de37f46c230be924032a87df730761

Thanks everyone for helping on the analysis and for reviews obviously.