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

Add --without-decimal-contextvar option to use just threads in decimal #83975

Open
skrah mannequin opened this issue Feb 29, 2020 · 11 comments
Open

Add --without-decimal-contextvar option to use just threads in decimal #83975

skrah mannequin opened this issue Feb 29, 2020 · 11 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@skrah
Copy link
Mannequin

skrah mannequin commented Feb 29, 2020

BPO 39794
Nosy @rhettinger, @gpshead, @mdickinson, @skrah
PRs
  • bpo-39794: Add --without-decimal-contextvar #18702
  • [3.8] bpo-39794: Add --without-decimal-contextvar (GH-18702) #18713
  • [3.7] bpo-39794: Add --without-decimal-contextvar (GH-18702) #18714
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/skrah'
    closed_at = None
    created_at = <Date 2020-02-29.11:24:13.696>
    labels = ['extension-modules', '3.8', 'type-bug', '3.7', '3.9']
    title = 'Add --without-decimal-contextvar option to use just threads in decimal'
    updated_at = <Date 2020-03-03.15:31:20.696>
    user = 'https://github.com/skrah'

    bugs.python.org fields:

    activity = <Date 2020-03-03.15:31:20.696>
    actor = 'skrah'
    assignee = 'skrah'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules']
    creation = <Date 2020-02-29.11:24:13.696>
    creator = 'skrah'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39794
    keywords = ['patch']
    message_count = 11.0
    messages = ['362971', '362972', '363003', '363016', '363019', '363092', '363111', '363150', '363154', '363163', '363268']
    nosy_count = 5.0
    nosy_names = ['rhettinger', 'gregory.p.smith', 'mark.dickinson', 'skrah', 'boytsovea']
    pr_nums = ['18702', '18713', '18714']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue39794'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 29, 2020

    bpo-39776 has shown that it is hard to understand the interaction between
    ContextVars and threading in embedded scenarios.

    I want to understand the code again, so I'm adding back a compile time
    option to enable the thread local context that was present prior to
    f13f12d.

    @skrah skrah mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir labels Feb 29, 2020
    @skrah skrah mannequin self-assigned this Feb 29, 2020
    @skrah skrah mannequin added type-bug An unexpected behavior, bug, or error 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir labels Feb 29, 2020
    @skrah skrah mannequin self-assigned this Feb 29, 2020
    @skrah skrah mannequin added the type-bug An unexpected behavior, bug, or error label Feb 29, 2020
    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 29, 2020

    Also, when I'm debugging things like bpo-39776, I don't want to
    switch between Python versions.

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 29, 2020

    New changeset 815280e by Stefan Krah in branch 'master':
    bpo-39794: Add --without-decimal-contextvar (bpo-18702)
    815280e

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 29, 2020

    New changeset 4d70124 by Stefan Krah in branch '3.8':
    [3.8] bpo-39794: Add --without-decimal-contextvar (GH-18702)
    4d70124

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 29, 2020

    New changeset c4ca1f8 by Stefan Krah in branch '3.7':
    [3.7] bpo-39794: Add --without-decimal-contextvar (GH-18702)
    c4ca1f8

    @gpshead
    Copy link
    Member

    gpshead commented Mar 1, 2020

    While i don't think this was needed in 3.7 and 3.8, you kept the default behavior the same so I don't think it is a problem.

    If someone builds an interpreter with this configure flag, does it break compatibility with anything that code may have started to expect as of 3.7?

    @rhettinger
    Copy link
    Contributor

    +1 for the backport. A build option isn't new feature, but it does provide a debugging tool and work-around for people who are having problems.

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Mar 2, 2020

    For the people who find this issue via a search engine:

    1. The defaults in 3.7, 3.8 and 3.9 are unchanged (you get
      the ContextVar).

    2. ./configure --without-decimal-contextvar enables the exact
      TLS context code from 3.3-3.6. There is no new code.

    3. On Windows you need to edit PC\pyconfig.h to enable the TLS
      context.

    4. There are extensive tests for the flag in:

      Modules/_decimal/tests/runall-memorydebugger.sh

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Mar 2, 2020

    If someone builds an interpreter with this configure flag, does it break
    compatibility with anything that code may have started to expect as of 3.7?

    Yes, anything that relies on the implicit context being coroutine-safe
    does not work. The only test that expectedly breaks in the stdlib is:

    Lib/test/test_asyncio/test_context.py

    Basically you can't use operators inside coroutines in the same thread
    if both coroutines use a different implicit context.

    You can of course replace all operators inside coroutines by the
    corresponding context methods:

       # Unsafe with TLS context:
       async def foo():
           with localcontext(Context(prec=10)):
               x = Decimal(1) / 9
    
       # Safe, just avoid the TLS context:
       async def foo():
           c = Context(prec=10)
           x = c.divide(Decimal(1), 9)

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Mar 2, 2020

    To make things more clear (it is good that Gregory asked):

    ONLY use this flag if you don't use coroutines at all
    OR control ALL async libraries in your application.

    Random async libraries from PyPI may rely on the
    ContextVar and silently give incorrect results.

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Mar 3, 2020

    bpo-39776 is fixed, but _pydecimal was also affected, see msg363266.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    Status: No status
    Development

    No branches or pull requests

    2 participants