bpo-43441: fix [Subinterpreters]: global variable next_version_tag cause method cache bug

[JunyiXie]

under building Python with --with-experimental-isolated-subinterpreters

when sub interpreter finalize.
_PyType_ClearCache set next_version_tag = 0.

Type shared between interpreters.
another interpreter assign_version_tag "1" for a type, the type is first assign.

the dealloc interpreter had assign_version_tag "1" for another type.

now, two different type has same version tag. it cause method cache wrong.

static unsigned int
_PyType_ClearCache(struct type_cache *cache)
{
#if MCACHE_STATS
    size_t total = cache->hits + cache->collisions + cache->misses;
    fprintf(stderr, "-- Method cache hits        = %zd (%d%%)\n",
            cache->hits, (int) (100.0 * cache->hits / total));
    fprintf(stderr, "-- Method cache true misses = %zd (%d%%)\n",
            cache->misses, (int) (100.0 * cache->misses / total));
    fprintf(stderr, "-- Method cache collisions  = %zd (%d%%)\n",
            cache->collisions, (int) (100.0 * cache->collisions / total));
    fprintf(stderr, "-- Method cache size        = %zd KiB\n",
            sizeof(cache->hashtable) / 1024);
#endif

    unsigned int cur_version_tag = next_version_tag - 1;
    next_version_tag = 0;
    type_cache_clear(cache, 0);

    return cur_version_tag;
}

https://bugs.python.org/issue43441

[vstinner]

This change leaks memory: type_cache_clear() clears objects and should be done in subinterpreters.

I suggest to pass interp to _PyType_ClearCache() and check _Py_IsMainInterpreter(interp) in this function to decide if next_version_tag must be modified or not.

[JunyiXie]

Thank you for your review, it is very reasonable. I have made the changes as you suggested.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

[JunyiXie]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

[vstinner]

Please remove the NEWS entry. I would prefer to keep --with-experimental-isolated-subinterpreters feature secret on purpose. It's still experimental (and broken).

[JunyiXie]

Okay, thanks for the hint.

[JunyiXie]

@vstinner Thank you for your code review, I have removed the news. But I don't know how to make bedevere/news — No news entry in Misc/NEWS.d/next/ or "skip news" label found check success.

[vstinner]

Hum, the cache parameter sounds redundant. Please remove it and get the cache with:

struct type_cache *cache = &interp->type_cache;

So you can remove struct type_cache *cache = get_type_cache(); in PyType_ClearCache().

[JunyiXie]

Thanks vstinner, good suggestion.

[JunyiXie]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

[vstinner]

It would be safer to have a per-interpreter next_version_tag. The problem is that there are still many static types which are shared between interpreters :-/ You may add a comment explaining that:

// bpo-42745: next_version_tag remains shared by all interpreters because of static types

Or put the comment on the variable definition:

// Used to set PyTypeObject.tp_version_tag
static unsigned int next_version_tag = 0;

[JunyiXie]

Thanks, ok i added

[JunyiXie]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

[vstinner]

I pushed my last requested change and I merged your PR, thanks for the fix.