Opened 6 years ago

Last modified 4 years ago

#5400 enhancement new

Change UDP port to have an explicit state machine, and no FileDescriptor dependency

Reported by: Itamar Turner-Trauring Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/filedescriptor-5400
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst

Description (last modified by Itamar Turner-Trauring)

FileDescriptor has way too much code, much of it irrelevant to simple cases like ports. It also implements an implicit, hard to test state machine, much of it irrelevant to ports.

As a starting point for replacing it, or at least restricting it to transports where all the extra logic is relevant, we should implement a UDP port that doesn't inherit from FileDescriptor, and uses an explicit state machine. The explicit state machine will help us learn the requirements for the more complex state machine we can expect for TCP transports.

Change History (8)

comment:1 Changed 6 years ago by itamarst

Author: itamarst
Branch: branches/filedescriptor-5400

(In [33202]) Branching to 'filedescriptor-5400'

comment:2 Changed 6 years ago by Itamar Turner-Trauring

Keywords: review added

OK, up for review. I went for the absolute minimum in order to ease code review; possibly there will eventually be a base class connectionLost, but I want to write some real code before going there.

I'm also not thinking about IOCP at all. Possibly that is a mistake? I'm thinking explicit state machines + common mixins/utility objects may be the way to go, but I don't understand that code sufficiently.

Next step will be trying to switch one of our ports to use this - any thoughts on whether it should be TCP or UDP? I'm thinking TCP just because it's the more complex use case, and because I want to fix #5368. And, can I change the base class for an existing Port, or should I create a new class and modify listenTCP? No doubt someone is doing something horrible with that code, but probably not by inheritance, but rather manipulating objects, so *any* change to listenTCP will affect those people.

buildbot.twistedmatrix.com/boxes-supported?branch=/branches/filedescriptor-5400

comment:3 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Itamar Turner-Trauring

Thanks. We are desperately overdue for refactoring in twisted.internet.

  1. It looks like this is a candidate for a roll up branch, since this first round of changes doesn't actually change any existing code. I might even go ahead and implement all the Port changes and then split the result into two pieces - it seems like it will be easy to separate the "provide a Port base class" from the "provide a concrete Port" changes pretty easily.
  2. It doesn't look like this FileDescriptor is actually particularly Port-oriented, though? Ports don't need to write, do they? They probably shouldn't implement IWriteDescriptor at all.
  3. Can we keep makeStatefulDispatcher private for a while longer? I don't think it's a particularly good thing - it served its purpose in _newclient.py pretty well, but it's not very clear how well it generalizes yet. At the very least, it would be nice for a public API like this to be based on #4669 instead of strings. Also twisted.python.statemachine seems like a misleading name, anyway. It's not really a state machine, it's a method call dispatcher that uses some state. :)

More generally, it would be really great (for me) to see a branch (or two branches) that actually employs the new code to simplify something or fix a bug somewhere before deciding that this new private abstraction is worth adding to trunk.

comment:4 Changed 6 years ago by Itamar Turner-Trauring

I broke TCP (easy to fix), and _fd obviously needs a lot less code in it... but UDP passes all but one test in twisted.test.test_udp and twisted.internet.test.test_udp.

comment:5 Changed 5 years ago by Itamar Turner-Trauring

Description: modified (diff)
Summary: Create a replacement FileDescriptor class that has less hardcoded logicChange UDP port to have an explicit state machine, and no FileDescriptor dependency

Old ticket description:

FileDescriptor has way too much code, much of it irrelevant to simple cases like ports. It also implements an implicit, hard to test state machine.

We should create a replacement that is simpler, and with hooks for an explicit state machine. Later tickets could then begin switching some our port classes to this new class, and someday even transports.

I decided to switch the ticket to the concrete case of switching UDP ports away from FileDescriptor.

comment:6 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: Itamar Turner-Trauring deleted

Putting up for partial review. I don't think it's quite ready for a detailed review, since I haven't done a cleanup pass, nor checked if additional tests are required. My hope is none are needed because I deliberately added no new features or bug fixes, only moved some code around, but I need to check if existing coverage is reasonable.

As a first pass I mostly want some architectural feedback - does the general design make sense? Is this a violation of the backwards compatibility policy? The module docstring says "don't use this", but there's no underscores.

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

Keywords: review removed
Owner: set to Itamar Turner-Trauring

I see a lot of things that look like mistakes, like udp.Port subclassing both FileDescriptor and ReadDescriptor. I guess I'll assume that those are the kinds of cleanup things that you expected, and won't comment on them. That gives me a free pass to say as much as I want about that sort of thing in the next review though. :)

As far as the general approaches goes... and having just mostly failed to review #5192, I'm not that happy with the makeStatefulDispatcher approach to state machines. It fails to capture explicit inputs, outputs, and transitions (ie, it only captures states). It also doesn't even make explicit the states that can exist.

Ideally, the resolution to this branch would make explicit: all the states that exist (and part of code, rather than only part of documentation - but thanks for documenting those); the inputs allowed by each; the outputs produced for those inputs; and the new states transitioned to as a consequence of receiving those inputs.

There isn't really anything in Twisted already that is implemented like this. So maybe look at this as a chance to invent some fun totally abstract infrastructure. >:)

comment:8 Changed 4 years ago by Itamar Turner-Trauring

Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone
Note: See TracTickets for help on using tickets.