some methods of uninitialized io.IncrementalNewlineDecoder objects raise SystemError

[orenmn]

BPO	31718
Nosy	@benjaminp, @serhiy-storchaka, @orenmn, @csabella, @ZackerySpytz
PRs	bpo-31718: Raise ValueError instead of SystemError when calling methods of uninitialized io.IncrementalNewlineDecoder objects #3913 bpo-31718: Fix io.IncrementalNewlineDecoder SystemErrors and segfaults #18640

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-10-06.18:09:48.760>
labels = ['type-bug', '3.8', '3.9', 'expert-IO']
title = 'some methods of uninitialized io.IncrementalNewlineDecoder objects raise SystemError'
updated_at = <Date 2020-02-24.06:50:50.454>
user = 'https://github.com/orenmn'

bugs.python.org fields:

activity = <Date 2020-02-24.06:50:50.454>
actor = 'ZackerySpytz'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['IO']
creation = <Date 2017-10-06.18:09:48.760>
creator = 'Oren Milman'
dependencies = []
files = []
hgrepos = []
issue_num = 31718
keywords = ['patch']
message_count = 6.0
messages = ['303842', '303847', '303853', '303860', '303874', '361625']
nosy_count = 6.0
nosy_names = ['benjamin.peterson', 'stutzbach', 'serhiy.storchaka', 'Oren Milman', 'cheryl.sabella', 'ZackerySpytz']
pr_nums = ['3913', '18640']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue31718'
versions = ['Python 3.8', 'Python 3.9']

[orenmn]

Given an uninitialized IncrementalNewlineDecoder:
uninitialized = io.IncrementalNewlineDecoder.__new__(io.IncrementalNewlineDecoder)

each of the following calls would raise a SystemError ('null argument to
internal routine'):
uninitialized.getstate()
uninitialized.setstate((b'foo', 0))
uninitialized.reset()

In contrast, the following call would raise a ValueError
('IncrementalNewlineDecoder.__init__ not called'):
uninitialized.decode(b'bar')

ISTM that getstate(), setstate(), and reset() should have the same behavior as
decode(). (Though i think that including the actual type name in the error
message would be better, as it could be a subclass of IncrementalNewlineDecoder).

[serhiy-storchaka]

Is IncrementalNewlineDecoder subclassable?

[orenmn]

Yes, although i don't know if there are usecases for that.

[serhiy-storchaka]

There is other issues with IncrementalNewlineDecoder.__init__ -- it leaks references when called repeatedly.

The simplest solution of both issues will be moving the initialization to the new method. But this class looks designed for subclassing, and this can break subclasses that change arguments before passing them to the superclass' constructor.

[orenmn]

With regard to refleaks in __init__() methods, i started looking for similar refleaks
in the codebase, and hope to open an issue to fix them soon.

[csabella]

The patch for this issue had some outstanding questions/changes to be made before being merged, but now a new PR needs to be opened to replace the original one. Anyone who is interested can pick this up, but please only open one PR for this and please also credit the original author as per the dev guide. Thank you!