bpo-28557: error message for bad raw readinto #7496

davidszotten · 2018-06-07T20:57:20Z

https://bugs.python.org/issue28557

davidszotten · 2018-06-07T20:57:25Z

Would prefer to still return an OSError with the TypeError as a cause, but couldn't figure out how to do that. Hints/help would be appreciated.

benjaminp

Yeah, I don't think you should change the exception type.

Modules/_io/bufferedio.c

Lib/test/test_io.py

bedevere-bot · 2019-09-12T13:47:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

benjaminp · 2019-09-12T16:57:06Z

So, I just noticed other similar places in BufferedReader have code like this:

    if (n < 0) {
        if (!PyErr_Occurred())
            PyErr_Format(PyExc_OSError,
                         "Raw stream returned invalid position %" PY_PRIdOFF,
                         (PY_OFF_T_COMPAT)n);
        return -1;
    }

So that will propagate the exception for PyNumber_AsSsize_t directly. I probably wouldn't have designed it this way, but we should follow that existing convention I think.

Sorry for the whiplash.

davidszotten · 2019-09-12T18:26:09Z

So that will propagate the exception for PyNumber_AsSsize_t directly.

Sorry, i'm not sure i quite follow what you mean here

benjaminp · 2019-09-13T12:49:14Z

I'm saying the error handling code should look like this.

davidszotten · 2019-09-13T18:53:42Z

i see your point about following conventions in the file but i think that makes this a lot less ergonomic and helpful for debugging

without cause:

Traceback (most recent call last):
  File "/Users/david/dev/cpython/try.py", line 7, in <module>
    bufio.readline()
OSError: raw readinto() returned invalid value

with cause:

TypeError: 'bytes' object cannot be interpreted as an integer

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/david/dev/cpython/try.py", line 7, in <module>
    bufio.readline()
OSError: raw readinto() returned invalid value

davidszotten · 2019-09-23T12:53:52Z

friendly bump @benjaminp

davidszotten · 2020-03-08T18:07:21Z

@csabella should we change this from awaiting changes to awaiting review again?

remilapeyre · 2020-06-06T21:26:47Z

Lib/test/test_io.py

Suggested change

assert cm.exception.__cause__ is None

self.assertIsNone(cm.exception.__cause__)

remilapeyre · 2020-06-06T21:27:08Z

Lib/test/test_io.py

Suggested change

assert isinstance(cm.exception.__cause__, TypeError)

self.assertIsInstance(cm.exception.__cause__, TypeError)

davidszotten · 2020-06-07T07:29:18Z

thanks for the suggestions @remilapeyre . updated

remilapeyre

Thanks @davidszotten ! Just one small nit, I'm pretty sure an error must be set when n == 1 so you don't have to check PyErr_Occurred() too. Otherwise it looks good to me!

remilapeyre · 2020-06-10T21:43:12Z

Modules/_io/bufferedio.c

    n = PyNumber_AsSsize_t(res, PyExc_ValueError);
    Py_DECREF(res);
+
+    if (n == -1 && PyErr_Occurred()) {


Suggested change

if (n == -1 && PyErr_Occurred()) {

if (n == -1) {

i don't quite understand why, but removing that check i get a segfault

here's the bottom of the stack from lldb:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) frame #0: 0x00000001001542f5 python.exe`_PyErr_FormatVFromCause [inlined] _Py_DECREF(op=0x0000000000000000) at object.h:441:9 [opt] frame #1: 0x00000001001542f5 python.exe`_PyErr_FormatVFromCause(tstate=0x0000000100506680, exception=0x0000000100292fc0, format="raw readinto() returned invalid value", vargs=0x00007ffeefbf9a40) at errors.c:586 [opt] * frame #2: 0x0000000100154460 python.exe`_PyErr_FormatFromCause(exception=<unavailable>, format=<unavailable>) at errors.c:626:5 [opt] frame #3: 0x00000001001e8508 python.exe`_bufferedreader_raw_read(self=<unavailable>, start=<unavailable>, len=8192) at bufferedio.c:1488:9 [opt]

hang on, i think i get it now. the point is that n is the return value of the (user-defined) readinto which means it can sometimes be -1 even without PyErr_Occurred. this is then caught in the next if block (and becomes "raw readinto() returned invalid length [..]")

the whole point of this pr is to help users debug bad readinto implementations by providing more granular error messages

Yes sorry, this was a mistake :/

davidszotten · 2020-06-11T13:41:50Z

@remilapeyre no worries

does that mean this is ready? pr is marked as "requested changes" from @benjaminp though he never replied to my comment about his suggested change

what's the next step?

remilapeyre · 2020-06-11T14:13:21Z

@remilapeyre no worries

does that mean this is ready? pr is marked as "requested changes" from @benjaminp though he never replied to my comment about his suggested change

what's the next step?

I think your change looks good, new it needs to be reviewed by a core developer like @benjaminp and they will either merge it or ask you for changes. There is a lot of work going on on Python and they have a lot of PR to review so it may take them some time but they will get there :)

davidszotten · 2020-06-11T15:58:33Z

is there a difference between the tags for "awaiting review" and "awaiting core review" that's applicable here?

remilapeyre

The first is when nobody reviewed, the second is when someone who is not a core developer has made a review. It should have switched but the bot seems to have missed it.

davidszotten · 2020-06-16T12:21:49Z

thank you!

the-knights-who-say-ni added the CLA signed label Jun 7, 2018

bedevere-bot added the awaiting review label Jun 7, 2018

davidszotten force-pushed the fix-issue-28557 branch from e6a4161 to 72339b2 Compare June 7, 2018 21:04

csabella requested a review from benjaminp May 17, 2019 23:58

benjaminp requested changes Sep 12, 2019

View reviewed changes

Modules/_io/bufferedio.c Show resolved Hide resolved

Modules/_io/bufferedio.c Outdated Show resolved Hide resolved

Lib/test/test_io.py Show resolved Hide resolved

bedevere-bot removed the awaiting review label Sep 12, 2019

bedevere-bot added the awaiting changes label Sep 12, 2019

davidszotten force-pushed the fix-issue-28557 branch 2 times, most recently from 5157566 to e72ab00 Compare September 12, 2019 16:36

csabella requested a review from benjaminp January 12, 2020 13:48

csabella added awaiting review and removed awaiting changes labels Mar 8, 2020

csabella requested review from benjaminp and removed request for benjaminp March 8, 2020 18:58

remilapeyre suggested changes Jun 6, 2020

View reviewed changes

bpo-28557: error message for bad raw readinto

2b8468e

davidszotten force-pushed the fix-issue-28557 branch from e72ab00 to 2b8468e Compare June 7, 2020 06:54

remilapeyre approved these changes Jun 10, 2020

View reviewed changes

remilapeyre approved these changes Jun 11, 2020

View reviewed changes

benjaminp added 2 commits June 15, 2020 18:31

fix indentation

69063e4

improve error message; it didn't return if it raised an exception

3864512

benjaminp merged commit 8666356 into python:master Jun 15, 2020

bedevere-bot removed the awaiting review label Jun 15, 2020

	assert cm.exception.__cause__ is None
	self.assertIsNone(cm.exception.__cause__)

	assert isinstance(cm.exception.__cause__, TypeError)
	self.assertIsInstance(cm.exception.__cause__, TypeError)

Uh oh!

bpo-28557: error message for bad raw readinto #7496

bpo-28557: error message for bad raw readinto #7496

Uh oh!

Conversation

davidszotten commented Jun 7, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidszotten commented Jun 7, 2018

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

benjaminp commented Sep 12, 2019

Uh oh!

davidszotten commented Sep 12, 2019

Uh oh!

benjaminp commented Sep 13, 2019

Uh oh!

davidszotten commented Sep 13, 2019

Uh oh!

davidszotten commented Sep 23, 2019

Uh oh!

davidszotten commented Mar 8, 2020

Uh oh!

remilapeyre Jun 6, 2020

Choose a reason for hiding this comment

Uh oh!

remilapeyre Jun 6, 2020

Choose a reason for hiding this comment

Uh oh!

davidszotten commented Jun 7, 2020

Uh oh!

remilapeyre left a comment

Choose a reason for hiding this comment

Uh oh!

remilapeyre Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

davidszotten Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

davidszotten Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

remilapeyre Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

davidszotten commented Jun 11, 2020

Uh oh!

remilapeyre commented Jun 11, 2020

Uh oh!

davidszotten commented Jun 11, 2020

Uh oh!

remilapeyre left a comment

Choose a reason for hiding this comment

Uh oh!

davidszotten commented Jun 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davidszotten commented Jun 7, 2018 •

edited by bedevere-bot

Loading