Opened 3 years ago

Closed 2 years ago

#5412 defect closed fixed (fixed)

pollingfile._PollableWritePipe should check for unicode earlier

Reported by: zseil Owned by: itamar
Priority: normal Milestone:
Component: core Keywords: windows
Cc: ivank-twisted-bugs@… Branch: branches/pollingfile-write-unicode-5412
(diff, github, buildbot, log)
Author: moijes12 Launchpad Bug:

Description

pollingfile._PollableWritePipe currently checks for outgoing unicode data in checkWork, its equivalent of FileDescriptor.doWrite. This is too late, the user's code will not be notified of this error, it will just break the _PollingTimer to which the pipe is attached. The check for unicode data should be made in write and writeSequence instead.

Attachments (3)

patch_5412.patch (3.4 KB) - added by moijes12 3 years ago.
The check for unicode has been removed from _PollableWrite.checkWork. Checks for unicode have been added to _PollableWritePipe.write and _PollableWritePipe.writeSequence. The unit tests have been added to test_pollingfile.TestPollableWritePipe and the test involving checkWork has been removed. Do note that I've created this patch using Eclipse as I'm trying to setup a work environment and if this causes any issues, please let me know. I'll make the required changes.
5412.patch (3.7 KB) - added by moijes12 2 years ago.
5412.2.patch (3.7 KB) - added by moijes12 2 years ago.

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by moijes12

The check for unicode has been removed from _PollableWrite.checkWork. Checks for unicode have been added to _PollableWritePipe.write and _PollableWritePipe.writeSequence. The unit tests have been added to test_pollingfile.TestPollableWritePipe and the test involving checkWork has been removed. Do note that I've created this patch using Eclipse as I'm trying to setup a work environment and if this causes any issues, please let me know. I'll make the required changes.

comment:1 Changed 3 years ago by moijes12

  • Keywords review added

comment:2 Changed 3 years ago by ivank

  • Cc ivank-twisted-bugs@… added
  • Keywords review removed
  • Owner set to moijes12

Thanks for the patch!

  1. Needs a NEWS file. See http://twistedmatrix.com/trac/wiki/ReviewProcess
  1. Because writeSequence accepts any sequence, including sequences that can only be iterated once (like a generator), you must not iterate over the sequence twice. (First, map iterates, then extend iterates again. The sequence could be empty the second time.)
  1. Please add a test for the above, so this doesn't get broken in the future.
  1. I think there's an extra colon here: @type seq: : A C{sequence}
  1. Please remove the trailing whitespace you added to many locations. Also, blank lines should have no whitespace/indent.
  1. Nit: please end your docstring sentences with a period.

Thanks again. I couldn't do a full review, so please put it back into review after you fix those things.

Also, I am uncertain as to what arguments write and writeSequence accept (does it take buffer objects too?). Perhaps the next reviewer could take a look.

comment:3 Changed 3 years ago by exarkun

  1. Because writeSequence accepts any sequence, including sequences that can only be iterated once (like a generator), you must not iterate over the sequence twice. (First, map iterates, then extend iterates again. The sequence could be empty the second time.)

This might be a nice thing to eventually handle, but twisted/internet/abstract.py iterates twice, so I think in practice we don't support these kinds of sequences.

Also, I am uncertain as to what arguments write and writeSequence accept (does it take buffer objects too?). Perhaps the next reviewer could take a look.

twisted/internet/abstract.py probably works with buffers, but I don't know if we have any test coverage for this. It would probably be good if it worked with buffer, but adding tests for that and making any necessary changes to get them to pass could be a separate ticket.

Changed 2 years ago by moijes12

comment:4 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

I've attached the patch. Its nearly identical to the previous patch except for the newsfile. Thats because going by the exarkun's comments, twisted doesn't support sequences which can be iterated over only once. I've added the tests and made the required formatting changes. I was unable to test this as I was unable to build twisted on a windows PC and the tests were skipped in Linux. I hope this is good enough.

comment:5 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/pollingfile-write-unicode-5412

(In [34061]) Branching to 'pollingfile-write-unicode-5412'

comment:6 Changed 2 years ago by exarkun

(In [34062]) Apply 5412.patch

refs #5412

comment:7 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner set to moijes12

Thanks for the updated patch!

  1. C{sequence} suggests there's some sort of Python object known as "sequence", which there isn't; the @type is therefore both wrong and unhelpful. Perhaps iterable or just plain sequence would be better, and omit the @type in writeSequence.
  2. There's a dangling period on a line by itself on line 256 of _pollingfile.py.
  3. You should have two lines of whitespace between methods; this is old code that predates that, probably, but since you're modifying it you should add least fix that for the methods you are touching.
  4. You probably want to say C{seq} on line 245 of _pollingfile (the @raise).
  5. Docstrings for write and writeSequence should talk about something like "Write bytes" and "Write a list or other sequences of bytes"; referring to "data" is too vague.
  6. The news file could omit all but the first of the references to twisted.intenet._pollingfile._PollableWritePipe, for brevity's sake ("checks for outgoing unicode in .write()" etc.)

You can run tests yourself using force-builds.py (see http://labs.twistedmatrix.com/2011/09/pre-trunk-testing.html), but unfortunately only if you have a branch; when you submit the final patch I will run them for you.

Changed 2 years ago by moijes12

comment:8 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

comment:9 Changed 2 years ago by moijes12

Going by the comments by itamar

  1. Points 1,2,3 and 6 done as suggested.
  2. Point 4 - No. I meant the input seq. I don't know what a C{seq} is. Please help.
  3. Point 5 - I've made the changes. But I'm afraid they'll still need more fine-tuning. Let me know what you think of them.

comment:10 Changed 2 years ago by itamar

  • Owner set to itamar

BTW, if you're providing patches and a branch already exists, please provide a patch against the branch.

"C{seq}" is epytext markup. It means the API documentation output will use a code styling for the output, which is useful in emphasizing that this you're referring to an input variable.

In any case, I've applied the patch, tweaked the docstring, and will do final pass if the tests pass:

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/pollingfile-write-unicode-5412

comment:11 Changed 2 years ago by itamar

  • Author changed from exarkun to moijes12
  • Keywords review removed

Looks good, will merge as soon as I see a green Windows buildslave.

comment:12 Changed 2 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(In [34212]) Merge pollingfile-write-unicode-5412: Check for unicode writes earlier in _pollingfile.

Author: moijes12
Review: ivank, exarkun, itamar
Fixes: #5412

Check for unicode writes earlier in pollingfile._PollableWritePipe, so it's clear which bit of user's code messed up.

Note: See TracTickets for help on using tickets.