bpo-31718: Raise ValueError instead of SystemError when calling methods of uninitialized io.IncrementalNewlineDecoder objects#3913
Conversation
| PyErr_SetString(PyExc_ValueError, | ||
| "IncrementalNewlineDecoder.__init__ not called"); | ||
| PyErr_Format(PyExc_ValueError, | ||
| "%.200s.__init__() not called", |
There was a problem hiding this comment.
Revert this change. The problem is that IncrementalNewlineDecoder.__init__ not called. This can happen if subclass' __init__ doesn't call super().__init__. Subclass's __init__ can be called, but self->decoder == NULL if IncrementalNewlineDecoder.__init__ not called.
| PyObject *errors) | ||
| /*[clinic end generated code: output=fbd04d443e764ec2 input=89db6b19c6b126bf]*/ | ||
| { | ||
| Py_XDECREF(self->decoder); |
There was a problem hiding this comment.
There is a race condition, destructor of self->decoder can call IncrementalNewlineDecoder.__init__ . Use Py_XSETREF().
|
Sorry about messing up so much. |
|
LGTM. I'll merge if @benjaminp doesn't have comments. |
benjaminp
left a comment
benjaminp
left a comment
There was a problem hiding this comment.
Two minor things:
- Most of the _io module classes abstract away the reinitialization logic with a macro
CHECK_INITIALIZED. It would be nice to do that here. - Checking for initialization should be the first thing a method does. In this implementation,
.reset()can partially reset the object before raising an exception.
|
Note that i added an |
|
Python implementation of I don't understand why the |
|
@benjaminp @serhiy-storchaka so should the C implementation allow |
csabella
requested review from
benjaminp and
serhiy-storchaka
and removed request for
benjaminp
|
@orenmn, please resolve the merge conflicts. Thank you! |
|
@csabella I am really sorry, but I am not working on CPython nowadays.. |
|
@orenmn, no problem and thanks for responding. I'm going to close this pull request for now so that someone else can continue with it if they are interested. If you come back to it later on and it's not resolved yet, feel free to reopen it at that time. Thanks again! |
|
It would be better to not close abandoned PRs which fix a bug or implement a valuable enhancement, but recreate them, update and merge. |
|
Sorry, but that was my point in closing the abandoned PRs, to allow someone the opportunity to do just that. If someone becomes aware that a PR exists but isn't being worked on, then it gives that person a chance to pick up the issue and continue it. Otherwise these PRs are just being left open and no one seems to be aware of them. I thought I was implementing a good workflow for the older issues, but perhaps it should be discussed more and then formalized? |
7 participants
In addition:
test_ioto verify that indeedValueErroris raised instead ofSystemError.io.IncrementalNewlineDecoder.__init__().https://bugs.python.org/issue31718