Opened 9 years ago

Last modified 7 years ago

#5413 task assigned

The producer/consumer implementation of pollingfile._PollableWritePipe has some bugs

Reported by: zseil Owned by: John Popplewell
Priority: normal Milestone:
Component: core Keywords: windows
Cc: Branch:
Author:

Description (last modified by Jean-Paul Calderone)

I started filling bugs with #5412, but then found #2839, #2835 and that:

  1. writeSequence doesn't pause the producer when outgoing buffer is full,
  2. data written with write gets dropped while the pipe is still disconnecting,
  3. bufferEmpty sets producerPaused attribute on the producer instead of on itself, consequently producer.resumeProducing can get called too many times.

File a ticket for each of these bugs as well. Then, fix them. This ticket is overly broad and should probably be closed as a duplicate once those tickets are filed.

Attachments (3)

test_pollingfile-all-6-new-tests.patch (18.3 KB) - added by John Popplewell 7 years ago.
_pollingfile-use-filedescriptor.patch (19.9 KB) - added by John Popplewell 7 years ago.
_pollingfile-using-filedescriptor-with-improved-tests.patch (29.9 KB) - added by John Popplewell 7 years ago.
Improved tests, smaller patch to _pollingfile

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by Jean-Paul Calderone

Note #5400.

comment:2 Changed 7 years ago by Jean-Paul Calderone

Description: modified (diff)
Summary: refactor pollingfile._PollableWritePipe as a subclass of abstract.FileDescriptorThe producer/consumer implementation of pollingfile._PollableWritePipe has some bugs

Changing the summary and description of this ticket. Composition should be preferred over inheritance in almost all cases, and this seems like a case where that wisdom clearly applies. However, the *problem* is several bugs in _PollableWritePipe. The implementation details of the solution to the problem may be a subject for discussion.

comment:3 Changed 7 years ago by John Popplewell

I've posted patches to the 3 existing tickets that add a test-case: #2835 #2839 #5365 and created 3 new tickets with patches attached that demonstrate the 3 bugs mentioned in this ticket: #6491 #6492 #6493.

I've got a patch (posted earlier) that shows how these bugs can be fixed by using abstract.FileDescriptor but it contained unnecessary changes related to adding Windows console support.

I believe I can make a minimalist patch that just fixes these bugs (using abstract.FileDescriptor) without any additional 'clutter'.

I would however attempt to address the under-documentation of _pollingfile.py.

I got pydoctor working and it makes very clear the deficiencies in the current code, and in my previous patch :-)

comment:4 Changed 7 years ago by Tom Prince

Owner: set to John Popplewell

johnnypops: I haven't looked at all your patches, but how you've handled them is a bit confusing. It appears that the patches just touch the tests. Any code that gets merged needs to have passing tests, so they can't be applied as is.

Your description makes it sound like you intend to submit one patch to fix all the tests you have submitted patches. If that is the case, you should submit a single patch that includes the tests and the code changes.

However, it would be better to make smaller incremental changes. I'm not sure if that is possible, and it isn't clear what code would be involved, so I can't look. In any case, if you have a number of related patches touching the same code that depend on each other, you should submit them for review one at a time: until the first is dealt with, the later tickets can't be merged, or meaningfully reviewed.

comment:5 in reply to:  4 Changed 7 years ago by John Popplewell

Replying to tom.prince:

johnnypops: I haven't looked at all your patches, but how you've handled them is a bit confusing. It appears that the patches just touch the tests. Any code that gets merged needs to have passing tests, so they can't be applied as is.

OK, that makes sense, I'm still struggling with the procedure.

Your description makes it sound like you intend to submit one patch to fix all the tests you have submitted patches. If that is the case, you should submit a single patch that includes the tests and the code changes.

I've got a single patch ready for all the tests (I actually broke the original down into separate tests for each bug :)

However, it would be better to make smaller incremental changes. I'm not sure if that is possible, and it isn't clear what code would be involved, so I can't look. In any case, if you have a number of related patches touching the same code that depend on each other, you should submit them for review one at a time: until the first is dealt with, the later tickets can't be merged, or meaningfully reviewed.

I understand, but I believe the situation is quite unusual, or at least awkward. I was working on adding Windows console support when I discovered the various tickets describing a total of 6 bugs in the existing _pollingfile implementation. They are all fixed by relatively minor changes to pollingfile (deriving from abstract.FileDescriptor, and implementing a few functions).

I tried the big patch approach but it wasn't anywhere near the required standard, documentation-wise as currently _pollingfile is almost entirely undocumented. I'm working on that right now, after being pointed in the right direction. The tests were also insufficient.

So I've kind of been running round in circles a bit :) The full patch should be available tomorrow some time and I guess I'll post it here.

comment:6 Changed 7 years ago by John Popplewell

Keywords: review added
Status: newassigned

Added a pair of patches. One adds 6 tests to test_pollingfile that exposes the following bugs in _pollingfile:

#2835 #2839 #5365 #6491 #6492 #6493

The second is a patch to _pollingfile that fixes all of the above bugs by deriving _PollableReader and _PollableWriter from abstract.FileDescriptor.

I added documentation to _pollingfile whilst I was in there.

Changed 7 years ago by John Popplewell

Changed 7 years ago by John Popplewell

comment:7 Changed 7 years ago by Tom Prince

Some comments from IRC, in case they get missed there:

<tomprince> johnnypops: Are the changes for the different bugs the same, or different?                                                                              
<tomprince> Or, better, can the changes for the different bugs be split up.
<tomprince> johnnypops: Is that really the smallest functional change you could make at once?
<tomprince> johnnypops: Looking at it, getting the documentation changes in on their own would probably be easy, and make the diff easier to review. (As a start)

Also, there is a lot of code here, with no explanation of *why* the code is changed the way it was. I'm not terribly familiar with win32, and most reviewers aren't (or would rather not be). So, as is, this isn't likely to get reviewed.

Also, have you read https://twistedmatrix.com/trac/wiki/ReviewProcess ?

comment:8 Changed 7 years ago by John Popplewell

Reworked tests: now using ReactorBuilder resulting in simpler and more readable tests which also exercise multiple reactors, thanks itamar, exarkun. Overall, test_pollingfile patch is down from about 550 to about 350 lines :-)

Also knocked about 100 lines off the patch to _pollingfile itself. There were still changes in there irrelevant for the current task.

My motivation for working on _pollingfile is that I'd like to get Windows console support working and my current solution reuses the machinery of _pollingfile by making it more flexible. It seems reasonable (to me) to fix outstanding bugs in _pollingfile, add documentation and clean it up a little first.

Inheriting from abstract.FileDescriptor may not be an ideal solution, but it does provide the (somewhat messy) logic for handling the producer/consumer loseConnection() logic. It also makes it easy to handle the non-blocking pipe behavior when an attempt to write a buffer larger than the current pipe size would fail - write what you can, put the rest back into the buffers.

Changed 7 years ago by John Popplewell

Improved tests, smaller patch to _pollingfile

comment:9 Changed 7 years ago by John Popplewell

Also resolves #5032, or at least makes the script run to completion: I saw "You will not see this." :-)

comment:10 Changed 7 years ago by John Popplewell

Keywords: review removed
Note: See TracTickets for help on using tickets.