Opened 9 years ago

Closed 7 years ago

#1780 defect closed fixed (fixed)

self.transport.pauseProducing() in connectionMade is ignored for clients

Reported by: itamarst Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: spiv, exarkun, therve, glyph, itamarst, radix, foom Branch: branches/pauseproducing-ignored-1780-2
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

at least for tcp, since the code in transport that calls protocol.makeConnection(self) immediately follows that with self.startReading().

This can e.g. make twisted.protocols.portforward break if it receives any data before it has succeeded connecting to destination.

Change History (34)

comment:1 Changed 9 years ago by spiv

  • Cc spiv added

comment:2 Changed 9 years ago by glyph

  • Summary changed from self.transport.pauseProducing() in connectionMade is ignored to self.transport.pauseProducing() in connectionMade is ignored for clients

This is only on the client side. Servers behave correctly.

comment:3 Changed 7 years ago by exarkun

  • Cc exarkun added
  • Priority changed from high to highest

comment:4 Changed 7 years ago by exarkun

(In [21301]) Merge epoll-hup-err-2809-2

Author: exarkun
Reviewer: therve
Fixes #2809
Refs #1780
Refs #1662
Refs #2833

Change the error (EPOLLHUP, EPOLLERR) checking code so that it can
handle the case where they are both set, rather than only checking
the cases where one or the other is set. This allows epollreactor
to properly detect and report connections which are lost uncleanly
and also are not being polled for input events.

The test case for this is slightly convoluted for a number of
reasons:

  • gtkreactor cannot pass the new test, and I am not fixing that reactor in this branch. The test will skip when that reactor is in use. #2833 was created as a result of this in order to be able to remove the broken gtkreactor eventually.
  • If a file descriptor is being polled for neither read nor write events, it will not be polled for hang-up events either. This is a known issue, #1662. When it is fixed, the test will need to be updated.
  • In order to exercise the case where the event reported from epoll is only HUP|ERR (specifically, not IN), the test pauses one of the involved transports. However, due to #1780, it must use a timing hack to accomplish this. Once that issue is resolved, it should be possible to remove this hack.

comment:5 Changed 7 years ago by therve

  • Cc therve added

I've looked quickly at this, and it doesn't seem very hard, but I see a problem: if we fix pauseProducing to just work in connectionMade, we might expect to break some stuff (I haven't check in Twisted itself).

I have one proposition, which would be to add a semantic to connectionMade: the returned value means whether you want to start reading immediately or not (with a special check for None, meaning True). Writing it, I realize that's not really a good idea, but I don't have another for now :). We might just as well fix pauseProducing, I don't know.

What do you think?

comment:6 Changed 7 years ago by exarkun

Regarding breaking existing code, do you have a particular kind of breakage in mind? The obvious case - where code calls pauseProducing in connectionMade and expects it to have no effect - doesn't seem like a reasonable expectation for code to have; that is, it strikes me as clearly buggy behavior, not behavior changes to which need to follow the backwards compatibility guidelines.

comment:7 Changed 7 years ago by itamarst

I agree with exarkun; the fact it doesn't work is a bug, there's no way a reasonable person would expect it *not* to work. Also, code that depends on the fact calling a random method is a noop at a given point seems unlikely.

comment:8 Changed 7 years ago by therve

  • Owner changed from glyph to therve
  • Type changed from enhancement to defect

OK, I asked because it doesn't look different to me from #1785 for example.

Now that we (almost) have a failing test, this is relatively easy. Doing that once #2813 is merged.

comment:9 Changed 7 years ago by therve

I've started to look at this, but my this solution involves adding a new flag attribute on BaseClient. Would it be an acceptable solution?

comment:10 Changed 7 years ago by exarkun

flags kind of suck, since often combinations of different flags are unexpected or untested. but, if a flag is the way to implement this, then I don't really object.

maybe you could describe what you encountered that makes you think a new flag is the way to solve this, though? someone might be able to suggest a simpler solution (I kind of expected this to be more or less resolved by re-ordering some framework and application code).

comment:11 Changed 7 years ago by therve

(In [21580]) The fix

Refs #1780

comment:12 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Hum you were right :). It's ready to review in pauseproducing-ignored-1780.

comment:13 Changed 7 years ago by exarkun

  • Cc glyph itamarst radix foom added

Cool, that's basically the fix I was expecting. :)

It'd be good to have an explicit unit test just for this case too, though.

I can't really think of a blackbox test that could be written for this. It's a test for the actual TCP transport, so it has to use the real TCP transport classes. That means it's subject to whatever decisions the platform TCP implementation wants to make. That means it's hard to be sure the data won't be delivered after pauseProducing is called (as opposed to it just not having been delivered yet). We could probably write something that was almost correct - send a lot of data, wait a second, see if any of it got delivered. Most (maybe even all extant) TCP implementations would deliver it fast enough for the test to notice it and fail within a pretty short period of time. Still, that's pretty crummy.

A whitebox test for this could just check the reactor to make sure the transport isn't being monitored for events. Is that better than a time-sensitive test? I'm not sure. I think so. But then it needs an implementation for each reactor. Maybe that's alright too, though.

If there were a public API for checking what events a... thing... (file descriptor? I dunno) is being monitored for, then that whitebox test would become a blackbox test, though.

comment:14 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Okay I guess no one else cares. I suggest doing a whitebox test for each reactor.

comment:15 Changed 7 years ago by therve

(In [22143]) Add new methods to check for fd monitoring, with a test.

Refs #1780

comment:16 Changed 7 years ago by therve

  • author set to therve
  • Branch set to branches/pauseproducing-ignored-1780
  • Keywords review added
  • Owner therve deleted

This is not totally ready to merge, but it deserves a first look to check if the solution is acceptable.

comment:17 follow-up: Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

I'd probably name isMonitoredForReading hasReader to mirror addReader and removeReader.

test_pauseProducingInConnectionMade is good, but maybe not quite as complete as it could be. An assertion that the monitored state is not tampered with after connectionMade (really makeConnection) returns would be a nice addition.

I'm not really sure what we want to do about adding new methods to IReactorFDSet. It's probably fine, but of course if there are any third-party implementations they'll be incomplete with this change.

comment:18 Changed 7 years ago by therve

(In [22187]) Rename to hasReader/hasWriter/hasFileDescriptor

Refs #1780

comment:19 Changed 7 years ago by therve

(In [22188]) Add a check after connectionMade

Refs #1780

comment:20 in reply to: ↑ 17 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Replying to exarkun:

I'd probably name isMonitoredForReading hasReader to mirror addReader and removeReader.

Done.

test_pauseProducingInConnectionMade is good, but maybe not quite as complete as it could be. An assertion that the monitored state is not tampered with after connectionMade (really makeConnection) returns would be a nice addition.

Done.

I'm not really sure what we want to do about adding new methods to IReactorFDSet. It's probably fine, but of course if there are any third-party implementations they'll be incomplete with this change.

That brings something: iocpreactor doesn't implement IReactorFDSet, so we can skip the test easily.

comment:21 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Switch to getReaders and getWriters as discussed on IRC. There should be tests for these as well, of course.

comment:22 Changed 7 years ago by therve

(In [22372]) Add getReader/getWriters, with tests.

Refs #1780

comment:23 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

comment:24 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to therve

comment:25 Changed 7 years ago by therve

(In [22377]) Process review comments.

Refs #1780

comment:26 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

(Latest commit was for #2921).

I have a little doubt of getReaders and getWriters docstring, but this is up for review.

comment:27 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Getting close, I think. I'm surprised getReaders and getWriters return a list of int, though. Having them return a list of IReadDescriptor and IWriteDescriptor seems more useful and simple to me.

The docstrings seem okay to. Maybe it would be better if they said something like "monitored for input events" instead, but it doesn't seem like much of a difference.

A very vaguely nice thing would be to have less duplication, but if that involves extensive refactoring or making the class hierarchy even more complex with mixins then it's not worthwhile.

comment:28 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Alright. Points 1 and 2 are handled. Regarding the duplication, this is indeed a bit unfortunate, but the simplest solution I've found involve mixins, so leaving like this for now.

comment:29 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Skip for PauseProducingTestCase.test_pauseProducingInConnectionMade would be nicer outside the test method (like the one for ReactorFDTestCase).

The cfreactor implementation of getReaders and getWriters might be simpler if they used keys (several other implementations do that, so I'm not sure if cfreactor was overlooked or intentionally excluded)?

It'd be nice to merge the branch forward and try it on buildbot. Right now there's a bunch of failures that are probably due to other stuff which has since been fixed.

If you're feeling very adventurous, then I think ReactorFDTestCase would be well expressed as a parameterized test case which uses a newly created reactor instead of the global one. testSuite works well enough now that we can start writing these, I think, and it would be good if we did. On the other hand, I don't think there's yet a public API for making all the necessary TestCase instances, so it might be better to have this as a separate task rather than part of this one.

comment:30 Changed 7 years ago by therve

  • Branch changed from branches/pauseproducing-ignored-1780 to branches/pauseproducing-ignored-1780-2

(In [22465]) Branching to 'pauseproducing-ignored-1780-2'

comment:31 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

I think the adventure will happen later :). Please review.

comment:32 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Great. Thanks! Please merge.

comment:33 Changed 7 years ago by therve

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

(In [22723]) Merge pauseproducing-ignored-1780-2

Author: therve
Reviewer: exarkun
Fixes #1780

Allow transport.pauseProducing to work in connectionMade on the client side.
Previously, due to internal mechanisms in tcp, this was incorrectly ignored.

To ease the tests, a new API of the reactor has been added, getReaders and
getWriters, that respectly returned the file descriptors monitored for reading
and writing by the reactor.

comment:34 Changed 4 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.