Issue2711

classification
Title: Dictionaries fail test of atomicity
Type: behaviour Severity: normal
Components: Core Versions: Jython 2.7
Milestone:
process
Status: closed Resolution: invalid
Dependencies: Superseder:
Assigned To: jeff.allen Nosy List: jeff.allen, zyasoft
Priority: normal Keywords: Java Roadmap

Created on 2018-10-25.22:45:49 by jeff.allen, last changed 2019-04-04.05:40:27 by jeff.allen.

Messages
msg12158 (view) Author: Jeff Allen (jeff.allen) Date: 2018-10-25.22:45:47
Tests in test_dict and test_dict_jy fail as follows on versions from Java 9. I'm raising this issue to track the failure, while inserting skips to turn the bots green.


======================================================================
FAIL: test_setdefault_atomic (__main__.PyStringMapTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\Jeff\Documents\Eclipse-O\jython-jvm9\dist\Lib\test\test_dict.py", line 339, in test_setdefault_atomic
    self.assertEqual(hashed1.eq_count + hashed2.eq_count, 1)
AssertionError: 2 != 1


======================================================================
FAIL: test_setdefault_atomic (__main__.DictTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\Jeff\Documents\Eclipse-O\jython-jvm9\dist\Lib\test\test_dict.py", line 339, in test_setdefault_atomic
    self.assertEqual(hashed1.eq_count + hashed2.eq_count, 1)
AssertionError: 2 != 1
msg12159 (view) Author: Jeff Allen (jeff.allen) Date: 2018-10-26.18:18:22
Skipped on Java 9+ for now in: https://hg.python.org/jython/rev/5aa64ce0b8b8
msg12410 (view) Author: Jeff Allen (jeff.allen) Date: 2019-03-29.08:08:54
I took a second look at this because new failures turned up during #2649. I conclude that it is not valid to expect this test to pass on Jython because it tests for a particular implementation, not for atomicity per se.

The original CPython bug is https://bugs.python.org/issue13521, based on a discussion where P J Eby reminds us dict.setdefault is atomic only if the __hash__ and __eq__ of all keys are defined in C. This is because the internal logic of testing for the key and adding an entry is atomic under the GIL. If that logic calls out to Python, the GIL is (potentially) released to another thread between Python instructions.

The change introduced at that time works (I think) by finding the proper location for the key (using hash and equality) only once and inserting there if necessary. The test actually uses a __hash__ (guaranteed to collide) and __eq__ that are implemented in Python, and tests that they are called only once.

ConcurrentHashMap, and PyDictionary based on it, fails this test, because it may call __eq__ repeatedly. However, it does this under a lock (on the bucket where the key belongs). It does not matter what the implementation of __eq__ is, no other thread gets to manipulate that bucket. (Bets are off, I suspect, if your __hash__ or __eq__ manipulate the container in the owning thread.)
msg12420 (view) Author: Jeff Allen (jeff.allen) Date: 2019-04-04.05:40:27
On the logic of the previous note, closing as invalid.
History
Date User Action Args
2019-04-04 05:40:27jeff.allensetstatus: open -> closed
resolution: invalid
messages: + msg12420
2019-03-29 08:08:54jeff.allensetkeywords: - test failure causes
assignee: jeff.allen
messages: + msg12410
nosy: + zyasoft
2018-10-26 18:18:22jeff.allensetmessages: + msg12159
2018-10-25 22:45:49jeff.allencreate