Implement exercise bank-account

[mrcfps]

Solves #730, alternative to #757.

Hi, I'm striving to attack the problem of concurrency testing from a different angle. As is documented in Thread State and the Global Interpreter Lock, there is only ever one thread per process active to execute Python bytecode, which, as a side effect, slashes the possibility of race condition.

But when Python switches threads, chances of execution breakup come, the culprit behind race condition. By default, Python 3 switches threads every 0.005 seconds (just try sys.getswitchinterval()) and Python 2 every 100 Python virtual instructions (try sys.getcheckinterval()), and that's much longer than the execution time of deposit or withdraw methods (around 100 ns). So I set the switch interval to a small number to make bank account methods not as "atomic" as they are before.

I hope my PR would help in tackling concurrency-related exercises in the Python track.

[cmccandless]

@mrcfps Were you able to confirm that removing the with self.lock lines causes tests to fail? This would be adequate proof for me that the sys.setswitchinterval() solves our concurrency issue.

@N-Parsons thoughts?

[mrcfps]

@cmccandless yes I'm 100% sure that only with synchronization can we pass the tests. To ensure statistical stability, I've taken the same approach as java test suite to test concurrent transaction for 10 times, so that the non-deterministic nature of multi-threading won't break our tests.

[mrcfps]

Need more insight? First get a feel of how Python execute multiple threads (from Understanding GIL, David Beazley):

The key takeaway is that only one thread can access our BankAccount instance at the same time, which is a quite different picture compared with other languages such as Java.

Then let's analyze Python bytecode.

BankAccount.deposit without a lock:

def deposit(self, amount):
    if self.is_open and amount > 0:
        self.balance += amount
    else:
        raise ValueError

Here is its disassembly:

>>> dis.dis(BankAccount.deposit)
 16           0 LOAD_FAST                0 (self)
              2 LOAD_ATTR                0 (is_open)
              4 POP_JUMP_IF_FALSE       30
              6 LOAD_FAST                1 (amount)
              8 LOAD_CONST               1 (0)
             10 COMPARE_OP               4 (>)
             12 POP_JUMP_IF_FALSE       30

 17          14 LOAD_FAST                0 (self)
             16 DUP_TOP
             18 LOAD_ATTR                1 (balance)
             20 LOAD_FAST                1 (amount)
             22 INPLACE_ADD
             24 ROT_TWO
             26 STORE_ATTR               1 (balance)
             28 JUMP_FORWARD             4 (to 34)

 19     >>   30 LOAD_GLOBAL              2 (ValueError)
             32 RAISE_VARARGS            1
        >>   34 LOAD_CONST               0 (None)
             36 RETURN_VALUE

To reproduce race condition, operations between 18 LOAD_ATTR (reading balance) and 26 STORE_ATTR (storing balance) have to be separated. To put it more concretely, if thread A has loaded self.balance and modified it, then another thread B should take over to corrupt the data before A reseizes control and stores the self.balance.

If thread switch time is the default 0.005s (in Python 3) or 100 instructions (in Python 2), then thread A will have more than enough time to execute all instructions above in a row before another thread takes control as if the whole deposit method is atomic. That's why in #757, absence of self.lock still doesn't incur race condition.

But when we set the switch interval (or check interval in Py2) to a pretty short time, then in the midst of retrieving, modifying and storing self.balance, thread A will have a fairly high chance to yield control to other threads due to regular thread switch, thus prone to data corruption.

With threading.Lock, all bytecode instructions in the deposit can be executed truly as a whole, keeping control of the process even if it's time for thread switch, just because all other threads cannot acquire the lock.

[cmccandless]

@mrcfps Very good analysis! I would still like to hear @N-Parsons thoughts on the subject as he had some things to say when the concurrency issue was raised before.

[N-Parsons]

@mrcfps @cmccandless

I just tried running this without the threading and lock, and it still passes in both Python 2 and Python 3. This seems to be because the concurrency test is starting with £1000 in the account and then adds and subtracts a small amount (£5).

After a fair bit of playing around, I was able to get failure to be fairly reproducible, but there are a few more issues that I think need to be looked at (noted at the bottom of this comment).

I've edited the code a little (initial balance of 0, sleep for 0.001s in the middle of the transaction, run it 100 times), and it now seems to fail every time, but I'm going to do a quick statistical analysis to optimise the number of runs and check that failure is sufficiently likely. I also want to check this on some VMs to make sure that reliable failure isn't core-count dependent.

Modified test:

import time
...
   def test_can_handle_concurrent_transactions(self):
        self.account.open()
        #self.account.deposit(0)

        for _ in range(100):
            self.adjust_balance_concurrently()

    def adjust_balance_concurrently(self):
        def transact():
            self.account.deposit(5)
            time.sleep(0.001)
            self.account.withdraw(5)

        # Greatly improve the chance of an operation being interuppted
        # by thread switch, thus testing synchronization effectively
        try:
            sys.setswitchinterval(1e-12)
        except AttributeError:
            # Python 2 compatible
            sys.setcheckinterval(1)

        threads = []
        for _ in range(1000):
            t = threading.Thread(target=transact)
            threads.append(t)
            t.start()

        for thread in threads:
            thread.join()

        self.assertEqual(self.account.get_balance(), 0)

Another issue:

Sometimes exceptions are raised from within the threads, but these don't currently cause the failure of the test. While I was playing around, I had a few tests pass that had exceptions raised within them - these need to be made to be failures. I also saw a few of these with failing tests with my updated test code above.

..Exception in thread Thread-6000:
Traceback (most recent call last):
  File "/usr/lib/python3.5/threading.py", line 914, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.5/threading.py", line 862, in run
    self._target(*self._args, **self._kwargs)
  File "bank_account_test.py", line 100, in transact
    self.account.withdraw(5)
  File "/tmp/exercism/python/exercises/bank-account/example.py", line 32, in withdraw
    raise ValueError
ValueError

..........
----------------------------------------------------------------------
Ran 12 tests in 1.520s

OK

[N-Parsons]

I played around a bit more and came up with this test implementation: https://gist.github.com/N-Parsons/cdb731944ad1e678fdd07d3b126ea853

My change has caused the test to fail every time for both Python 2 and Python 3 regardless of whether we're using locking or not - this is because the lock exists only within one operation but doesn't do anything to help when a new thread tries to withdraw money from an empty account that would have money in it if the previous thread had been allowed to finish and make the deposit.

My understanding of what we should be aiming for with concurrency is that we should be able to run another thread if we're waiting for something on another thread, but that it should never result in different behaviour to if we were running it without concurrency.

[mrcfps]

@N-Parsons I have followed your advice to tweak the test. I just add a single line of time.sleep(0.001), and it works out pretty well, showing a quite significant error due to the absence of lock. And here's my modifed test. Feel free to play around. If you find it ok, I'll push a commit.

The explanation for the thread ValueError issue you've encountered is that you initialized the balance to 0, so it's easy for a thread to withdraw more money than the account actually has. So I will stick to setting initial balance to 1000.

[mrcfps]

@cmccandless @N-Parsons Sorry for my poor branching strategy. I've been committing on the master branch, so it's pretty tricky to make new contributions. I'll re-fork this repo and work on a new branch.