BUG: Allow IOErrors when attempting to retrieve default client encoding. #21531

JayOfferdahl · 2018-06-18T18:26:25Z

closes BUG: mod_wsgi restricting access to stdout throws unhandled IOError. #21552

When using mod_wsgi, access to sys.stdout is restricted by default. To handle this case, catch a base exception instead of a more specific AttributeError during this process to include the thrown IOError by mod_wsgi.

…ing. instead of a more specific AttributeError to include the thrown IOError.

WillAyd · 2018-06-18T18:34:16Z

Is this referencing a particular issue?
Is there any other option besides catching base Exception (tend to avoid this if we can)
Can you add tests?

JayOfferdahl · 2018-06-18T18:39:02Z

Is this referencing a particular issue?
- I haven't raised an issue for this as it just came up this morning.
Is there any other option besides catching base Exception (tend to avoid this if we can)
- We could instead widen the exception clause to just AttributeError and IOError.
Can you add tests?
- Will do.

WillAyd · 2018-06-18T19:02:30Z

OK thanks - if you could open an issue and reference it then that would be preferable (typically PRs are only allowed without an issue if they are DOC updates).

You'll also want a whatsnew note to describe the bug and reference the issue. Next minor release is 0.23.2 at this point

codecov · 2018-06-18T21:15:06Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4e0b636). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #21531   +/-   ##
=========================================
  Coverage          ?   92.18%           
=========================================
  Files             ?      169           
  Lines             ?    50739           
  Branches          ?        0           
=========================================
  Hits              ?    46774           
  Misses            ?     3965           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.59% <100%> (?)`
#single	`42.34% <0%> (?)`

Impacted Files	Coverage Δ
pandas/io/formats/console.py	`75.75% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e0b636...81547da. Read the comment docs.

gfyoung · 2018-06-18T23:54:05Z

pandas/io/formats/console.py

@@ -21,7 +21,7 @@ def detect_console_encoding():
    encoding = None
    try:
        encoding = sys.stdout.encoding or sys.stdin.encoding
-    except AttributeError:
+    except Exception:


Why can't we just except both on AttributeError and IOError ? I generally try to avoid using the broad Exception unless we have good reason to.

) * When using mod_wsgi, access to sys.stdout is restricted by default. To handle this case, catch IOErrors alongside the already handled AttributeErrors.

jreback

looks ok, getting to pass travis might be a bit tricky.

jreback · 2018-06-20T10:12:33Z

pandas/tests/io/formats/test_console.py

+    assert detect_console_encoding() == 'foo'
+
+
+def test_detect_console_encoding_stdin(monkeypatch):


can you add some comments on what is being tested here.

jreback · 2018-06-20T10:12:48Z

doc/source/whatsnew/v0.23.2.txt

@@ -65,7 +65,7 @@ Bug Fixes
 **I/O**

 - Bug in :func:`read_csv` that caused it to incorrectly raise an error when ``nrows=0``, ``low_memory=True``, and ``index_col`` was not ``None`` (:issue:`21141`)
-
+- Bug in :meth:`detect_client_encoding` where potential IOError goes unhandled when importing in a mod_wsgi process due to restricted access to stdout. (:issue:`21552`)


double backticks on IOError

…JayOfferdahl/pandas into get-client-encoding-on-mod-wsgi

pep8speaks · 2018-06-20T13:35:15Z

Hello @JayOfferdahl! Thanks for updating the PR.

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

Comment last updated on August 23, 2018 at 03:18 Hours UTC

JayOfferdahl · 2018-06-20T13:36:15Z

Thanks for the feedback, made some changes to tests to cover more cases and provide some insight as to what they're doing. Is there anything special I need to do to get this working with travis?

jreback · 2018-06-21T00:21:18Z

looks like it might be tricky to monkeypatch how you are doing it: https://travis-ci.org/pandas-dev/pandas/jobs/394566147

does this test run locally?

JayOfferdahl · 2018-06-21T05:03:55Z

Interesting...it runs locally just fine...I'll probably need to patch stdout/stdin like the other tests and provide the property on that patched object.

JayOfferdahl · 2018-06-26T01:13:34Z

I've updated the test to mock out stdout/stdin instead of trying to change their encoding attribute. I do the same in other tests so I'm assuming this will work fine with travis.

Apologies for the slow turn around on this one!

WillAyd · 2018-06-26T04:03:41Z

pandas/tests/io/formats/test_console.py

+            super(MockEncoding, self).__init__()
+            self.encoding = encoding
+
+    monkeypatch.setattr('sys.{}'.format(empty), MockEncoding(''))


Any chance you saw the context manager for monkeypatch? Seems to be preferred for patching standard library items (may require a version bump for pytest dependency).

…JayOfferdahl/pandas into get-client-encoding-on-mod-wsgi

JayOfferdahl · 2018-07-13T03:03:59Z

@WillAyd I've taken a look at the context manager and have updated these tests. Looks like I missed the 0.23.2 release...my own fault, of course. Should I put my note in the 0.24 whatsnew document? Not sure how you all handle bugs in non-minor release versions.

Side note: I did have to upgrade pytest to 3.6.3, I'm almost certain that this will break travis since it won't have that version (maybe I'm wrong) -- where does this get specified?

WillAyd

@JayOfferdahl no worries on timing - we tentatively have a 0.23.3 bug fix release that you can move the whatsnew entry to. Looks like you also have some LINTing errors to address

@jreback do you have any opposition to increasing the min version of pytest to 3.6? It's a relatively new release but the context manager for monkey patching I think is a useful and clean feature

WillAyd · 2018-07-13T13:56:57Z

pandas/tests/io/formats/test_console.py

+    ['stdout', 'stdin']
+])
+def test_detect_console_encoding_from_stdout_stdin(monkeypatch, empty, filled):
+    # Ensures that when sys.stdout.encoding or sys.stdin.encoding is used when


Can add a reference to the issue for this and the other new tests, i.e. # GH 21552

WillAyd · 2018-07-13T13:57:28Z

pandas/tests/io/formats/test_console.py

+    lambda: 'ascii'
+])
+def test_detect_console_encoding_fallback_to_locale(monkeypatch, stdEncoding):
+    monkeypatch.setattr('locale.getpreferredencoding', lambda: 'foo')


Can put this in the context manager, no?

jreback · 2018-07-13T14:22:21Z

no problem bumping pytest version (separate PR)

JayOfferdahl · 2018-07-25T18:14:43Z

Inline with the class name gave the same error...ended up putting this in the docstring. This should still be just as easy to discover, I assume.

WillAyd · 2018-07-25T21:30:53Z

pandas/tests/io/formats/test_console.py

+
+class MockEncoding(object):
+    """
+    TODO(py27): replace with mock


I didn't see the LINT errors before but maybe you got it mixed up with the one I've commented on below? I still think inline with the class block opening would be preferable. Certainly a nit, but I haven't seen TODOs embedded in docstrings like this before

WillAyd · 2018-07-25T21:31:13Z

pandas/tests/io/formats/test_console.py

+    @property
+    def encoding(self):
+        return raiseOrReturn(self.val)
+


The current LINT error is because you are missing a blank line here

WillAyd · 2018-07-25T21:31:43Z

pandas/tests/io/formats/test_console.py

+    def encoding(self):
+        return raiseOrReturn(self.val)
+
+def raiseOrReturn(val):


Prefer PEP8 naming, so let's call this raise_or_return

WillAyd · 2018-07-25T21:32:02Z

pandas/tests/io/formats/test_console.py

+        assert detect_console_encoding() == filled
+
+
+@pytest.mark.parametrize('stdEncoding', [


PEP8 so std_encoding or simply encoding

* It might be apparent what I styleguide I use!!

JayOfferdahl · 2018-07-26T16:23:13Z

Changes made. I was using the regular pylint package and not flake8, which explains the difference. Apologies, and thanks for spending so much time on this, I really appreciate it.

WillAyd · 2018-07-26T16:25:11Z

pandas/tests/io/formats/test_console.py

+        return raise_or_return(self.val)
+
+
+def raise_or_return(val):


Can this be moved as an instance method of MockEncoding?

I'm also using it down below to test when defaulting to system default -- it covers the same goal of throwing an exception like above here. I could make it a static method of MockEncoding, but I thought using that object for both cases wouldn't make sense since the encoding property is only being consumed from accesses by stdout or stdin. What do you think?

@WillAyd did you see this?

Let's make this a class method just so all of the relevant constructs stay together

WillAyd

2 minor comments

WillAyd · 2018-08-03T15:34:17Z

pandas/tests/io/formats/test_console.py

+        return raise_or_return(self.val)
+
+
+def raise_or_return(val):


Let's make this a class method just so all of the relevant constructs stay together

WillAyd · 2018-08-03T15:34:57Z

pandas/tests/io/formats/test_console.py

+
+@pytest.mark.parametrize('std,locale', [
+    [MockEncoding('ascii'), lambda: 'ascii'],
+    [MockEncoding('ascii'), lambda: raise_or_return(Exception)],


Instead of mixing usage of raise_or_return here for each parameter just do it in the body of the function; should work for all parameters since in case of a string it returns the string anyway

WillAyd

lgtm so long as it goes green. @jreback

WillAyd · 2018-08-13T19:18:44Z

@JayOfferdahl can you rebase?

JayOfferdahl · 2018-08-16T02:14:08Z

Of course, is the general work flow to just squash all of my changes down into one commit and rebase on top of master or should I merge master into this?

WillAyd · 2018-08-16T02:26:06Z

Merge master in

WillAyd · 2018-08-17T17:03:53Z

doc/source/whatsnew/v0.24.0.txt

@@ -673,7 +673,7 @@ I/O

 - :func:`read_html()` no longer ignores all-whitespace ``<tr>`` within ``<thead>`` when considering the ``skiprows`` and ``header`` arguments. Previously, users had to decrease their ``header`` and ``skiprows`` values on such tables to work around the issue. (:issue:`21641`)
 - :func:`read_excel()` will correctly show the deprecation warning for previously deprecated ``sheetname`` (:issue:`17994`)
-
+- Bug in :meth:`detect_client_encoding` where potential ``IOError`` goes unhandled when importing in a mod_wsgi process due to restricted access to stdout. (:issue:`21552`)


Maybe move to 0.23.5? @jreback

Is this a regression? If not, I would probably stay with 0.24.0 unless there is a compelling reason to backport-patch this bug.

WillAyd

lgtm @jreback

JayOfferdahl · 2018-09-05T00:28:01Z

Any update here?

gfyoung

Sorry that this fell through the cracks! This looks good to merge from me as well.

cc @jreback

jreback

do we have other console tests that can go in here?

jreback · 2018-09-18T13:26:45Z

this is fine. can you rebase, and ping on green (@WillAyd / @gfyoung ok to merge then)

…ng-on-mod-wsgi

…JayOfferdahl/pandas into get-client-encoding-on-mod-wsgi

WillAyd · 2018-09-19T14:16:20Z

Thanks @JayOfferdahl !

JayOfferdahl · 2018-09-19T20:17:47Z

Thank you, and thanks for the help throughout the process!

…ng. (pandas-dev#21531)

BUG: Except IOErrors when attempting to retrieve default client encod…

6c2987d

…ing. instead of a more specific AttributeError to include the thrown IOError.

JayOfferdahl changed the title ~~BUG: Except IOErrors when attempting to retrieve default client encoding.~~ BUG: Allow IOErrors when attempting to retrieve default client encoding. Jun 18, 2018

gfyoung reviewed Jun 18, 2018

View reviewed changes

gfyoung added Bug IO Data IO issues that don't fit into a more specific label labels Jun 18, 2018

JayOfferdahl and others added 3 commits June 19, 2018 22:19

BUG: Allow IOErrors when attempting to retrieve client encoding. (#21552

c31b914

) * When using mod_wsgi, access to sys.stdout is restricted by default. To handle this case, catch IOErrors alongside the already handled AttributeErrors.

Merge branch 'pending-branch' into get-client-encoding-on-mod-wsgi

510f4b1

Merge branch 'master' into get-client-encoding-on-mod-wsgi

1afda5f

jreback requested changes Jun 20, 2018

View reviewed changes

JayOfferdahl added 2 commits June 20, 2018 08:33

Fixup tests to add more scenarios. Code review changes.

b37cb75

Merge branch 'get-client-encoding-on-mod-wsgi' of https://github.com/…

0b1ca1d

…JayOfferdahl/pandas into get-client-encoding-on-mod-wsgi

PEP8 changes.

bf26adb

JayOfferdahl and others added 2 commits June 25, 2018 20:11

Mock out stdout/stdin instead of trying to access their properties.

77a95d2

Merge branch 'master' into get-client-encoding-on-mod-wsgi

2598595

WillAyd reviewed Jun 26, 2018

View reviewed changes

JayOfferdahl and others added 3 commits July 12, 2018 21:57

Use context manager when monkeypatching standard library items.

59e0993

Merge branch 'get-client-encoding-on-mod-wsgi' of https://github.com/…

8b51b34

…JayOfferdahl/pandas into get-client-encoding-on-mod-wsgi

Merge branch 'master' into get-client-encoding-on-mod-wsgi

984da5c

WillAyd requested changes Jul 13, 2018

View reviewed changes

WillAyd requested changes Jul 25, 2018

View reviewed changes

Fix silly linting errors. Use pep8.

49ee7b5

* It might be apparent what I styleguide I use!!

WillAyd reviewed Jul 26, 2018

View reviewed changes

WillAyd requested changes Aug 3, 2018

View reviewed changes

Move raise_or_return to MockEncoding; clean up parameters.

40b3588

WillAyd approved these changes Aug 4, 2018

View reviewed changes

WillAyd mentioned this pull request Aug 13, 2018

Fix Python2 test failures in certain locales #22213

Merged

4 tasks

Merge branch 'master' into get-client-encoding-on-mod-wsgi

5e3c8af

WillAyd reviewed Aug 17, 2018

View reviewed changes

Merge branch 'master' into get-client-encoding-on-mod-wsgi

f485223

WillAyd approved these changes Aug 24, 2018

View reviewed changes

gfyoung approved these changes Sep 5, 2018

View reviewed changes

jreback requested changes Sep 5, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Sep 8, 2018

jreback approved these changes Sep 18, 2018

View reviewed changes

JayOfferdahl added 2 commits September 18, 2018 08:56

Merge remote-tracking branch 'upstream/master' into get-client-encodi…

ab67024

…ng-on-mod-wsgi

Merge branch 'get-client-encoding-on-mod-wsgi' of https://github.com/…

81547da

…JayOfferdahl/pandas into get-client-encoding-on-mod-wsgi

WillAyd merged commit bf29988 into pandas-dev:master Sep 19, 2018

JayOfferdahl deleted the get-client-encoding-on-mod-wsgi branch September 19, 2018 20:18

aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018

BUG: Allow IOErrors when attempting to retrieve default client encodi…

73ff71e

…ng. (pandas-dev#21531)

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

BUG: Allow IOErrors when attempting to retrieve default client encodi…

a51e7f1

…ng. (pandas-dev#21531)

		assert detect_console_encoding() == 'foo'


		def test_detect_console_encoding_stdin(monkeypatch):

		assert detect_console_encoding() == filled


		@pytest.mark.parametrize('stdEncoding', [

BUG: Allow IOErrors when attempting to retrieve default client encoding. #21531

BUG: Allow IOErrors when attempting to retrieve default client encoding. #21531

Conversation

JayOfferdahl commented Jun 18, 2018 • edited by WillAyd Loading

WillAyd commented Jun 18, 2018

JayOfferdahl commented Jun 18, 2018

WillAyd commented Jun 18, 2018

codecov bot commented Jun 18, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jun 20, 2018 • edited Loading

Comment last updated on August 23, 2018 at 03:18 Hours UTC

JayOfferdahl commented Jun 20, 2018

jreback commented Jun 21, 2018

JayOfferdahl commented Jun 21, 2018

JayOfferdahl commented Jun 26, 2018

Choose a reason for hiding this comment

JayOfferdahl commented Jul 13, 2018

WillAyd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jul 13, 2018

JayOfferdahl commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayOfferdahl commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

WillAyd commented Aug 13, 2018

JayOfferdahl commented Aug 16, 2018

WillAyd commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

JayOfferdahl commented Sep 5, 2018

gfyoung left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback commented Sep 18, 2018

WillAyd commented Sep 19, 2018

JayOfferdahl commented Sep 19, 2018

JayOfferdahl commented Jun 18, 2018 •

edited by WillAyd

Loading

codecov bot commented Jun 18, 2018 •

edited

Loading

pep8speaks commented Jun 20, 2018 •

edited

Loading

WillAyd left a comment •

edited

Loading