Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The format context mananger is not threadsafe #83

Open
walrusVision opened this issue Sep 12, 2018 · 3 comments
Open

The format context mananger is not threadsafe #83

walrusVision opened this issue Sep 12, 2018 · 3 comments

Comments

@walrusVision
Copy link

Using the format context manager in a threaded environment introduces a race condition where the global formatter and global plural flag can be reset by one context manager after another context manager has set it, and before it has formatted a string within that context.

A solution to this is bitmath should either make sure that the context manager is threadsafe, or warn in the documentation that the context manager isn't threadsafe and that a user should use the Bitmath object's format function directly instead.

How to REPRODUCE the issue:
This test case can reproduce the issue. The test is only checking format_string but a similar method could be used to test format_plural

#! /usr/bin/env python

from __future__ import print_function

import random
import queue
import threading
import time
import unittest

import bitmath

default_format = bitmath.format_string
bitmath.format_string = "{not_in_context_manager}"

THREAD_COUNT = 8

def format_string(event, err_queue):
    size = bitmath.KiB(0)
    try:
        with bitmath.format(fmt_str=default_format):
            event.wait()
            time.sleep(random.random() * 2)
            str(size)
    except Exception as e:
        err_queue.put(e)


class TestThreading(unittest.TestCase):

    def test_context_manager(self):
        event = threading.Event()
        err_queue = queue.Queue()
        threads = []
        for i in xrange(THREAD_COUNT):
            threads.append(threading.Thread(target=format_string, args=(event, err_queue)))
        for thread in threads:
            thread.start()
        time.sleep(1)
        event.set()
        for thread in threads:
            thread.join()

        error = err_queue.get()
        if error:
            raise error

if __name__ == '__main__':
    unittest.main()

And will produce the following error

======================================================================
ERROR: test_context_manager (__main__.TestThreading)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./test_threading.py", line 47, in test_context_manager
    raise error
KeyError: 'not_in_context_manager'

How REPRODUCIBLE (every time? intermittently? only in certain environments?):
In general the the error would only happen in threaded environments where there is a requirement to override the default formatter using the context manager. In our case it was happening intermittently when testing a threaded part of our application which was logging the size of bitmath objects

What you EXPECTED to happen:
For strings to be successfully formatted with the provided formatter to the context

What ACTUALLY happened:
Some sizes where formatted with the formatter of another context.

VERSION of bitmath effected:

  • Version: 1.3.1.1
  • Install Source: PyPi

Your OPERATING SYSTEM and the affected PYTHON VERSION:
unbuntu16.04 and python 2.7.14

@tbielawa
Copy link
Owner

@walrusVision Thank you for filing a detailed bug report and for following the template. Deep down inside I think I knew that probably wasn't thread-safe, thanks for confirming it. The first thing I'll do is add a warning to the documentation.

tbielawa added a commit that referenced this issue Sep 13, 2018
This comes from GitHub issue #83.

Using bitmath context-managers, including the plural formatter, is not
guaranteed to format instances as desired and may cause errors.

Workaround included in doc update.
@tbielawa
Copy link
Owner

There we go, https://bitmath.readthedocs.io/en/latest/module.html#context-managers added a note there.

Going to leave this bug open for the time being. Maybe I or someone else will have an idea on how to make that work in threads.

@walrusVision
Copy link
Author

Thanks @tbielawa!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants