EHN: Improved thread safety for read_html() GH16928
State: closed

 - [X] closes #16928
 - [x] tests added / passed
 - [X] passes ``git diff upstream/master --name-only -- '*.py' | flake8 --diff`` (On Windows, ``git diff upstream/master -u -- "*.py" | flake8 --diff`` might work as an alternative.)
 - [X] whatsnew entry

It failed a total of 6 tests when running `test_fast.sh` but failed none when running `pytest pandas/tests/io/test_html.py`.
It seems that these 6 failed tests are unrelated to my changes since they also occur on the master branch.

Labels:
name: Bug
name: Multithreading
description: Parallelism in pandas
name: IO HTML
description: read_html, to_html, Styler.apply, Styler.applymap

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=h1) Report
> Merging [#16930](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/6000c5b9624fdd8925099f215eba282bfbef87ce?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/16930/graphs/tree.svg?token=eZ4WkYLtcO&src=pr&width=650&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #16930      +/-   ##
==========================================
- Coverage   90.99%   90.97%   -0.02%     
==========================================
  Files         161      161              
  Lines       49303    49303              
==========================================
- Hits        44863    44854       -9     
- Misses       4440     4449       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.74% <100%> (ø)` | :arrow_up: |
| #single | `40.19% <0%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree#diff-cGFuZGFzL2lvL2h0bWwucHk=) | `84.85% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.71% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=footer). Last update [6000c5b...7688010](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=h1) Report
> Merging [#16930](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/4efe6560e07f28de6a1834fa90e31cef31b0fb18?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/16930/graphs/tree.svg?height=150&src=pr&token=eZ4WkYLtcO&width=650)](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #16930      +/-   ##
==========================================
- Coverage   91.05%   91.01%   -0.05%     
==========================================
  Files         161      161              
  Lines       49350    49350              
==========================================
- Hits        44936    44915      -21     
- Misses       4414     4435      +21
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.78% <100%> (-0.03%)` | :arrow_down: |
| #single | `40.26% <0%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree#diff-cGFuZGFzL2lvL2h0bWwucHk=) | `84.85% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `63.23% <0%> (-1.82%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=footer). Last update [4efe656...c01caaf](https://codecov.io/gh/pandas-dev/pandas/pull/16930?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Regarding adding the test.

I have trouble creating a test that reproduces the issue.
The following code snippet works fine when I run `pytest test_html.py::test_importcheck_thread_safety` but as soon as I run `pytest test_html.py` the test always passes. Neither of the threads throw an exception in that case.
```python
def get_html_file():
    filename = os.path.join(DATA_PATH, 'valid_markup.html')
    read_html(filename)


class ErrorThread(threading.Thread):
    def run(self):
        try:
            threading.Thread.run(self)
        except Exception as e:
            self.err = e
        else:
            self.err = None

def test_importcheck_thread_safety():
    t1 = ErrorThread(target = get_html_file)
    t2 = ErrorThread(target = get_html_file)
    t1.start()
    t2.start()
    while(t1.is_alive() or t2.is_alive()):
        pass
    assert None == t1.err == t2.err, "Errors when run in parallel"
```
> Neither of the threads throw an exception in that case.

I'm confused...isn't that what you want (because it doesn't fail)?
The threads don't throw exceptions regardless of whether I include my fix or not. Meaning the test I wrote is useless and would indicate that everything is fine while actual programs are crashing.
> The threads don't throw exceptions regardless of whether I include my fix or not.

Ah, okay.  That makes sense.  Does your code from the issue raise an Exception if you incorporate it as a test (just as a starter)?
I'm not sure what you mean by starter. I tried using the code from the issue directly earlier and it didn't work. Apparently exceptions need to be thrown by the main-thread.
By starter, I meant: does your code from the original issue form a valid test for our code-base (when incorporated into the `tests` directory?  You indeed answered the question.
@3553x : As I'm not sure at this point why `pytest` is behaving the way it is for you, how about this: you need the test to break and then pass (with your changes) on the build machines before we merge.  On a separate branch, add your test, push it, and have Travis and/or Appveyor (either one is sufficient) run and see if it breaks *without your changes*.  Then push your changes to that branch and see if they then pass.

If it does, commit your test and push to this PR.  Otherwise, let us know if it worked or not.  How does that sound to you?
That sounds like something worth trying out. I'll let you know once I'm done.
One other thing, if this operation is successful, if you could provide us to the links to the failing and passing builds.  That way we can see the differential between having and not having your patch.
I tried using travis with my test added on top of the unfixed version. Some builds fail because of linting, however they all seem to pass pytest. I realised that my earlier version might have passed since `_IMPORTS` is set to true after the first call to `read_html()`. So I imported `pandas.io.html` and reloaded it. But this didn't fix the issue. This change was included in the version that was tested by travis.

These are the additions that were made to `test_html.py`:
```python
try:
    from importlib import reload
except ImportError:
    pass
[...]
import pandas.io.html
[...]
class ErrorThread(threading.Thread):
    def run(self):
        try:
            super(ErrorThread, self).run()
        except Exception as e:
            self.err = e
        else:
            self.err = None

@pytest.mark.slow
def test_importcheck_thread_safety():
    reload(pandas.io.html)
    filename = os.path.join(DATA_PATH, 'valid_markup.html')
    helper_thread1 = ErrorThread(target = read_html, args = (filename,))
    helper_thread2 = ErrorThread(target = read_html, args = (filename,))
    helper_thread1.start()
    helper_thread2.start()
    while(helper_thread1.is_alive() or helper_thread2.is_alive()):
        pass
    assert None == helper_thread1.err == helper_thread2.err
```
Output of `pytest test_html.py::test_importcheck_thread_safety`:
```
=========================== test session starts ============================
platform linux -- Python 3.6.1, pytest-3.1.1, py-1.4.34, pluggy-0.4.0
rootdir: /home/fe/code/pandas, inifile: setup.cfg
plugins: xdist-1.16.0
collected 8 items 

test_html.py F

================================= FAILURES =================================
______________________ test_importcheck_thread_safety ______________________

    @pytest.mark.slow
    def test_importcheck_thread_safety():
        reload(pandas.io.html)
        filename = os.path.join(DATA_PATH, 'valid_markup.html')
        helper_thread1 = ErrorThread(target = read_html, args = (filename,))
        helper_thread2 = ErrorThread(target = read_html, args = (filename,))
        helper_thread1.start()
        helper_thread2.start()
        while(helper_thread1.is_alive() or helper_thread2.is_alive()):
            pass
>       assert None == helper_thread1.err == helper_thread2.err
E       AssertionError: assert None == ImportError('lxml not found, please install it',)
E        +  where None = <ErrorThread(Thread-1, stopped 140146488710912)>.err
E        +  and   ImportError('lxml not found, please install it',) = <ErrorThread(Thread-2, stopped 140146480318208)>.err

test_html.py:961: AssertionError
========================= 1 failed in 7.14 seconds =========================
```
Output of `pytest test_html.py`
```
=========================== test session starts ============================
platform linux -- Python 3.6.1, pytest-3.1.1, py-1.4.34, pluggy-0.4.0
rootdir: /home/fe/code/pandas, inifile: setup.cfg
plugins: xdist-1.16.0
collected 74 items 

test_html.py ..........................................................................

======================== 74 passed in 50.77 seconds ========================
```
@3553x you can add your test to the PR.
Hello @3553x! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on July 21, 2017 at 14:55 Hours UTC
pls rebase on master

```
git rebase -i upstream/master
git push yourremote yourbranch -f
```
@3553x : @jreback has some minor comments that if you could address, we will be able to merge this!
@3553x : Thanks for updating!

@jreback : this LGTM, but I'll leave it to you to check one more time before merging.
thanks @3553x