Skip to content

Unit string parsing is not thread-safe #11225

Description

@bmerry

Description

If multiple threads try to parse a unit string at the same time it can lead to errors (details below). I originally thought that this was caused by #11221, but even with a fix for that (#11224) I still see the error.

Expected behavior

Threads should be able to independently parse unit strings without locking.

Actual behavior

Running the script below sporadicly produces this traceback:

Traceback (most recent call last):
  File "/home/bmerry/src/astropy/astropy/units/format/generic.py", line 562, in _do_parse
    return cls._parse_unit(s, detailed_exception=False)
  File "/home/bmerry/src/astropy/astropy/units/format/generic.py", line 485, in _parse_unit
    raise ValueError()
ValueError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/bmerry/src/astropy/astropy/units/format/generic.py", line 565, in _do_parse
    return cls._parser.parse(s, lexer=cls._lexer, debug=debug)
  File "/home/bmerry/src/astropy/astropy/extern/ply/yacc.py", line 333, in parse
    return self.parseopt_notrack(input, lexer, debug, tracking, tokenfunc)
  File "/home/bmerry/src/astropy/astropy/extern/ply/yacc.py", line 1201, in parseopt_notrack
    tok = call_errorfunc(self.errorfunc, errtoken, self)
  File "/home/bmerry/src/astropy/astropy/extern/ply/yacc.py", line 192, in call_errorfunc
    r = errorfunc(token)
  File "/home/bmerry/src/astropy/astropy/units/format/generic.py", line 443, in p_error
    raise ValueError()
ValueError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/bmerry/src/astropy/astropy/units/core.py", line 1850, in __call__
    return f.parse(s)
  File "/home/bmerry/src/astropy/astropy/units/format/generic.py", line 547, in parse
    result = cls._do_parse(s, debug=debug)
  File "/home/bmerry/src/astropy/astropy/units/format/generic.py", line 570, in _do_parse
    raise ValueError(f"Syntax error parsing unit '{s}'")
ValueError: Syntax error parsing unit 'm / s'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./astropy-constants-threading.py", line 29, in <module>
    main()
  File "./astropy-constants-threading.py", line 26, in main
    raise excs[0]
  File "./astropy-constants-threading.py", line 11, in thread_func
    u.Unit('m / s')
  File "/home/bmerry/src/astropy/astropy/units/core.py", line 1870, in __call__
    raise ValueError(msg)
ValueError: 'm / s' did not parse as unit: Syntax error parsing unit 'm / s' If this is meant to be a custom unit, define it with 'u.def_unit'. To have it recognized inside a file reader or other code, enable it with 'u.add_enabled_units'. For details, see https://docs.astropy.org/en/latest/units/combining_and_defining.html

Steps to Reproduce

Run the following code (it might need to be run several times to manifest the error). It tries to parse 'm / s' in multiple threads. The issue doesn't occur when parsing a simple unit (e.g. 'm'), so it's probably something related to the general parser.

#!/usr/bin/env python3

import sys
import threading

import astropy.units as u


def thread_func(excs):
    try:
        u.Unit('m / s')
    except Exception as exc:
        excs.append(exc)


def main():
    sys.setswitchinterval(1e-6)
    excs = []
    threads = [threading.Thread(target=thread_func, args=(excs,))
               for i in range(32)]
    for thread in threads:
        thread.start()
    for thread in threads:
        thread.join()
    if excs:
        raise excs[0]


main()

System Details

I'm running my branch of astropy from #11224 to eliminate #11221 as a possible source for the error.

Linux-5.4.0-59-generic-x86_64-with-glibc2.29
Python 3.8.5 (default, Jul 28 2020, 12:59:40) 
[GCC 9.3.0]
Numpy 1.19.1
astropy 4.3.dev440+g8b22d3ccb
Scipy 1.5.2
Matplotlib 3.3.0

Activity

  1. bmerry commented on Jan 8, 2021

    @bmerry
    ContributorAuthor

    I dug through things a bit more and identified two issues:

    1. The parser object stores state in itself during parsing. So it's not safe for two threads to parse at the same time using the same parser.
    2. According to the comments in yacc.py, it's not safe for more than one thread to construct a parser at the same time. One way that could happen is due to Lazy properties are not threadsafe #11221, but there is the more subtle issue that there are multiple parsers in astropy and even with Lazy properties are not threadsafe #11221 fixed, threads might construct unrelated parsers at the same time.

    The first can be solved with a lock associated with each parser, but the second one will need a global lock shared by all parsers within astropy. My suggestion is that we put some wrappers around PLY to make it thread safe. Any preferences for where such wrapping should go? Maybe a new utils.parsing module?

    Incidentally, I'm not worried about the overheads of locking on single-threaded code. I measured the performance impact on Unit('m / s') and it's down in the noise.

  2. added a commit that references this issue on Jan 8, 2021
    85bffd9
  3. added 4 commits that reference this issue on Feb 21, 2021
    db01351
    e702505
    3c1627b
    d26efbf
  4. added a commit that references this issue on Feb 28, 2021
    362d6a6
  5. added a commit that references this issue on Mar 9, 2021
    2bd3f6f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions