On Mon, Apr 27, 2015 at 8:26 PM, Itamar Turner-Trauring <itamar@itamarst.org
wrote:
self.in_send_message seems like a potential source of bugs (can't see it being set to False on all branches here) and is also likely unnecessary.
self.in_send_message is redundant with self.waiting_id, which was added later, and will be refactored out next iteration. But, after a message removed from the queue and sent, we can't send another until either we get an ack message back, or the timeout fires. Those bits of code omitted here, are what resets self.in_send_message back to False. (Or eventually, self.waiting_id to None). Suggestions on a better way to do this are most welcome. I thought about only having check_for_send push itself back on the reactor loop if the queue is empty, and having the ack and/or timeout code fire check_for_send in other cases, instead of setting the variable. Not sure that's terribly simpler, though.
General advice:
1. Simplify, simplify, simplify.
2. Unit tests (see https://twistedmatrix.com/documents/current/core/howto/trial.html).
There actually is a test around this, although ugly, as it has to simulate the protocol and the factory's queue. It passes. This also works when we run integration tests with some 3rd party open source apps, but breaks in production at one location. #1 well taken though, I should break check_for_send up and make it easier to write simple tests.