From e7c37c4ec5a31402ba4d3fd95f2ee31d72183a83 Mon Sep 17 00:00:00 2001 From: Thom Nichols Date: Fri, 4 Jun 2010 17:00:51 -0400 Subject: [PATCH 1/6] connect uses the new function-on-state-transition so when the connect method returns you are guaranteed to be either in the 'connected' or 'disconnected' state. Could remove the 'connecting' state except uses it. --- sleekxmpp/xmlstream/xmlstream.py | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/sleekxmpp/xmlstream/xmlstream.py b/sleekxmpp/xmlstream/xmlstream.py index cc5e1ec..6dbe7b3 100644 --- a/sleekxmpp/xmlstream/xmlstream.py +++ b/sleekxmpp/xmlstream/xmlstream.py @@ -101,19 +101,16 @@ class XMLStream(object): def connect(self, host='', port=0, use_ssl=None, use_tls=None): "Establish a socket connection to the given XMPP server." - if not self.state.transition('disconnected','connecting'): - logging.warning("Can't connect now; Already in state %s", self.state.current_state()) + + if not self.state.transition('disconnected','connected', + func=self.connectTCP, args=[host, port, use_ssl, use_tls] ): + + if self.state['connected']: logging.debug('Already connected') + else: logging.warning("Connection failed" ) return False - try: - return self.connectTCP(host, port, use_ssl, use_tls) - finally: - # attempt to ensure once a connection attempt starts, we leave either in the - # 'connected' or 'disconnected' state. Otherwise the connect method is not reentrant - if self.state['connecting']: - if not self.state.transition('connecting','disconnected'): - logging.error("Couldn't return to the 'disconnected' state after connection failure!") - + logging.debug('Connection complete.') + return True # TODO currently a caller can't distinguish between "connection failed" and # "we're already trying to connect from another thread" @@ -148,9 +145,6 @@ class XMLStream(object): self.socket.connect(self.address) self.filesocket = self.socket.makefile('rb', 0) - if not self.state.transition('connecting','connected'): - logging.error( "State transition error!!!! Shouldn't have happened" ) - logging.debug('connect complete.') return True except socket.error as serr: From 66cf0c20218657622a5d98aee155df1ea37d9a89 Mon Sep 17 00:00:00 2001 From: Thom Nichols Date: Mon, 7 Jun 2010 13:16:02 -0400 Subject: [PATCH 2/6] context manager is working but there's a fatal flaw: inside the body of the 'with' statement, there's no way to tell whether or not the transition occurred or timed out. --- sleekxmpp/xmlstream/statemachine.py | 83 ++++++++++++++++++++++------- tests/test_statemachine.py | 57 ++++++++++++++++++++ 2 files changed, 122 insertions(+), 18 deletions(-) diff --git a/sleekxmpp/xmlstream/statemachine.py b/sleekxmpp/xmlstream/statemachine.py index 0c841a9..51b4aae 100644 --- a/sleekxmpp/xmlstream/statemachine.py +++ b/sleekxmpp/xmlstream/statemachine.py @@ -63,20 +63,22 @@ class StateMachine(object): return self.transition_any( (from_state,), to_state, wait=wait, func=func, args=args, kwargs=kwargs ) + def transition_any(self, from_states, to_state, wait=0.0, func=None, args=[], kwargs={} ): ''' Transition from any of the given `from_states` to the given `to_state`. ''' - with self.lock: - for state in from_states: - if isinstance(state,tuple) or isinstance(state,list): - raise ValueError( "State %s should be a string. Did you mean to call 'StateMachine.transition_any()?" % str(state) ) - if not state in self.__states: - raise ValueError( "StateMachine does not contain from_state %s." % state ) - if not to_state in self.__states: - raise ValueError( "StateMachine does not contain to_state %s." % to_state ) + if not (isinstance(from_states,tuple) or isinstance(from_states,list)): + raise ValueError( "from_states should be a list or tuple" ) + for state in from_states: + if not state in self.__states: + raise ValueError( "StateMachine does not contain from_state %s." % state ) + if not to_state in self.__states: + raise ValueError( "StateMachine does not contain to_state %s." % to_state ) + + with self.lock: start = time.time() while not self.__current_state in from_states: # detect timeout: @@ -102,23 +104,34 @@ class StateMachine(object): return False + def transition_ctx(self, from_state, to_state, wait=0.0): + if not from_state in self.__states: + raise ValueError( "StateMachine does not contain from_state %s." % state ) + if not to_state in self.__states: + raise ValueError( "StateMachine does not contain to_state %s." % to_state ) + + return _StateCtx(self, from_state, to_state, wait) + + def ensure(self, state, wait=0.0): ''' Ensure the state machine is currently in `state`, or wait until it enters `state`. ''' return self.ensure_any( (state,), wait=wait ) + def ensure_any(self, states, wait=0.0): ''' Ensure we are currently in one of the given `states` ''' - with self.lock: - for state in states: - if isinstance(state,tuple) or isinstance(state,list): - raise ValueError( "State %s should be a string. Did you mean to call 'StateMachine.transition_any()?" % str(state) ) - if not state in self.__states: - raise ValueError( "StateMachine does not contain state %s." % state ) + if not (isinstance(states,tuple) or isinstance(states,list)): + raise ValueError('states arg should be a tuple or list') + for state in states: + if not state in self.__states: + raise ValueError( "StateMachine does not contain state '%s'" % state ) + + with self.lock: start = time.time() while not self.__current_state in states: # detect timeout: @@ -132,6 +145,11 @@ class StateMachine(object): self.transition(self.__current_state, self._default_state) + def _set_state(self, state): #unsynchronized, only call internally after lock is acquired + self.__current_state = state + return state + + def current_state(self): ''' Return the current state name. @@ -146,12 +164,41 @@ class StateMachine(object): ''' return self.__current_state == state + +class _StateCtx: + + def __init__( self, state_machine, from_state, to_state, wait ): + self.state_machine = state_machine + self.from_state = from_state + self.to_state = to_state + self.wait = wait + self._timeout = False + def __enter__(self): - self.lock.acquire() - return self + self.state_machine.lock.acquire() + start = time.time() + while not self.state_machine[ self.from_state ]: + # detect timeout: + if time.time() >= start + self.wait: + logging.debug('StateMachine timeout while waiting for state: %s', self.from_state ) + self._timeout = True # to indicate we should not transition + break + self.state_machine.lock.wait(self.wait) + + logging.debug('StateMachine entered context in state: %s', + self.state_machine.current_state() ) + return self.state_machine def __exit__(self, exc_type, exc_val, exc_tb): - self.lock.nofityAll() - self.lock.release() + if exc_val is not None: + logging.exception( "StateMachine exception in context, remaining in state: %s\n%s:%s", + self.state_machine.current_state(), exc_type.__name__, exc_val ) + elif not self._timeout: + logging.debug(' ==== TRANSITION %s -> %s', + self.state_machine.current_state(), self.to_state) + self.state_machine._set_state( self.to_state ) + + self.state_machine.lock.notifyAll() + self.state_machine.lock.release() return False # re-raise any exception diff --git a/tests/test_statemachine.py b/tests/test_statemachine.py index fec3109..4cfb50d 100644 --- a/tests/test_statemachine.py +++ b/tests/test_statemachine.py @@ -197,6 +197,63 @@ class testStateMachine(unittest.TestCase): self.assertTrue(s['two']) + def testContextManager(self): + + s = sm.StateMachine(('one','two','three')) + + with s.transition_ctx('one','two'): + self.assertTrue( s['one'] ) + self.failIf( s['two'] ) + + #successful transition b/c no exception was thrown + self.assertTrue( s['two'] ) + self.failIf( s['one'] ) + + # failed transition because exception is thrown: + try: + with s.transition_ctx('two','three'): + raise Exception("boom!") + self.fail('exception expected') + except: pass + + self.failIf( s.current_state() in ('one','three') ) + self.assertTrue( s['two'] ) + + def testCtxManagerTransitionFailure(self): + + s = sm.StateMachine(('one','two','three')) + + with s.transition_ctx('two','three') as _s: + self.assertTrue( _s['one'] ) + self.failIf( _s.current_state in ('two','three') ) + + self.assertTrue( _s['one'] ) + + def r1(): + print 'thread 1 started' + self.assertTrue( s.transition('one','two') ) + print 'thread 1 transitioned' + + def r2(): + print 'thread 2 started' + self.failIf( s['two'] ) + with s.transition_ctx('two','three', 10) as _s: + self.assertTrue( _s['two'] ) + print 'thread 2 will transition on exit from the context manager...' + self.assertTrue( s['three'] ) + + t1 = threading.Thread(target=r1) + t2 = threading.Thread(target=r2) + + t2.start() # this should block until r1 goes + time.sleep(1) + t1.start() + + t1.join() + t2.join() + + self.assertTrue( s['three'] ) + suite = unittest.TestLoader().loadTestsFromTestCase(testStateMachine) From 47f1fb16909d3baaec5822b1dcbca89491d0d18c Mon Sep 17 00:00:00 2001 From: Thom Nichols Date: Mon, 7 Jun 2010 13:43:37 -0400 Subject: [PATCH 3/6] context manager now returns a boolean 'result' as the context variable to indicate whether the transition timed out or if you are actually locked when entering the context body --- sleekxmpp/xmlstream/statemachine.py | 4 ++-- tests/test_statemachine.py | 15 +++++++++------ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/sleekxmpp/xmlstream/statemachine.py b/sleekxmpp/xmlstream/statemachine.py index 51b4aae..b1ab757 100644 --- a/sleekxmpp/xmlstream/statemachine.py +++ b/sleekxmpp/xmlstream/statemachine.py @@ -182,12 +182,12 @@ class _StateCtx: if time.time() >= start + self.wait: logging.debug('StateMachine timeout while waiting for state: %s', self.from_state ) self._timeout = True # to indicate we should not transition - break + return False self.state_machine.lock.wait(self.wait) logging.debug('StateMachine entered context in state: %s', self.state_machine.current_state() ) - return self.state_machine + return True def __exit__(self, exc_type, exc_val, exc_tb): if exc_val is not None: diff --git a/tests/test_statemachine.py b/tests/test_statemachine.py index 4cfb50d..0173ff0 100644 --- a/tests/test_statemachine.py +++ b/tests/test_statemachine.py @@ -223,11 +223,12 @@ class testStateMachine(unittest.TestCase): s = sm.StateMachine(('one','two','three')) - with s.transition_ctx('two','three') as _s: - self.assertTrue( _s['one'] ) - self.failIf( _s.current_state in ('two','three') ) + with s.transition_ctx('two','three') as result: + self.failIf( result ) + self.assertTrue( s['one'] ) + self.failIf( s.current_state in ('two','three') ) - self.assertTrue( _s['one'] ) + self.assertTrue( s['one'] ) def r1(): print 'thread 1 started' @@ -237,10 +238,12 @@ class testStateMachine(unittest.TestCase): def r2(): print 'thread 2 started' self.failIf( s['two'] ) - with s.transition_ctx('two','three', 10) as _s: - self.assertTrue( _s['two'] ) + with s.transition_ctx('two','three', 10) as result: + self.assertTrue( result ) + self.assertTrue( s['two'] ) print 'thread 2 will transition on exit from the context manager...' self.assertTrue( s['three'] ) + print 'transitioned to %s' % s.current_state() t1 = threading.Thread(target=r1) t2 = threading.Thread(target=r2) From 9464736551311c015f1511e7535c762923838eaf Mon Sep 17 00:00:00 2001 From: Thom Nichols Date: Mon, 7 Jun 2010 13:58:15 -0400 Subject: [PATCH 4/6] added __str__ --- sleekxmpp/xmlstream/statemachine.py | 4 ++++ tests/test_statemachine.py | 7 +++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/sleekxmpp/xmlstream/statemachine.py b/sleekxmpp/xmlstream/statemachine.py index b1ab757..704cabd 100644 --- a/sleekxmpp/xmlstream/statemachine.py +++ b/sleekxmpp/xmlstream/statemachine.py @@ -163,6 +163,10 @@ class StateMachine(object): Use `StateMachine.ensure(state)` to wait until the machine enters a certain state. ''' return self.__current_state == state + + def __str__(self): + return "".join(( "StateMachine(", ','.join(self.__states), "): ", self.__current_state )) + class _StateCtx: diff --git a/tests/test_statemachine.py b/tests/test_statemachine.py index 0173ff0..00e4d3a 100644 --- a/tests/test_statemachine.py +++ b/tests/test_statemachine.py @@ -20,11 +20,14 @@ class testStateMachine(unittest.TestCase): # self.failIf(s.two) self.failIf(s['two']) try: - s.booga + s['booga'] self.fail('s.booga is an invalid state and should throw an exception!') except: pass #expected exception - + # just make sure __str__ works, no reason to test its exact value: + print str(s) + + def testTransitions(self): "Test ensure transitions occur correctly in a single thread" s = sm.StateMachine(('one','two','three')) From 34dc236126d272a16df0915b010dca0e2e38f0d4 Mon Sep 17 00:00:00 2001 From: Thom Nichols Date: Mon, 7 Jun 2010 14:41:42 -0400 Subject: [PATCH 5/6] added documentation for transition_ctx and removed some superfluous comment lines --- sleekxmpp/xmlstream/statemachine.py | 26 ++++++++++++++++++++++++++ tests/test_statemachine.py | 5 ----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/sleekxmpp/xmlstream/statemachine.py b/sleekxmpp/xmlstream/statemachine.py index 704cabd..c6e0ea4 100644 --- a/sleekxmpp/xmlstream/statemachine.py +++ b/sleekxmpp/xmlstream/statemachine.py @@ -105,6 +105,32 @@ class StateMachine(object): def transition_ctx(self, from_state, to_state, wait=0.0): + ''' + Use the state machine as a context manager. The transition occurs on /exit/ from + the `with` context, so long as no exception is thrown. For example: + + :: + + with state_machine.transition_ctx('one','two', wait=5) as locked: + if locked: + # the state machine is currently locked in state 'one', and will + # transition to 'two' when the 'with' statement ends, so long as + # no exception is thrown. + print 'Currently locked in state one: %s' % state_machine['one'] + + else: + # The 'wait' timed out, and no lock has been acquired + print 'Timed out before entering state "one"' + + print 'Since no exception was thrown, we are now in state "two": %s' % state_machine['two'] + + + The other main difference between this method and `transition()` is that the + state machine is locked for the duration of the `with` statement (normally, + after a `transition() occurs, the state machine is immediately unlocked and + available to another thread to call `transition()` again. + ''' + if not from_state in self.__states: raise ValueError( "StateMachine does not contain from_state %s." % state ) if not to_state in self.__states: diff --git a/tests/test_statemachine.py b/tests/test_statemachine.py index 00e4d3a..e44b8e4 100644 --- a/tests/test_statemachine.py +++ b/tests/test_statemachine.py @@ -15,9 +15,7 @@ class testStateMachine(unittest.TestCase): def testDefaults(self): "Test ensure transitions occur correctly in a single thread" s = sm.StateMachine(('one','two','three')) -# self.assertTrue(s.one) self.assertTrue(s['one']) -# self.failIf(s.two) self.failIf(s['two']) try: s['booga'] @@ -31,12 +29,9 @@ class testStateMachine(unittest.TestCase): def testTransitions(self): "Test ensure transitions occur correctly in a single thread" s = sm.StateMachine(('one','two','three')) -# self.assertTrue(s.one) self.assertTrue( s.transition('one', 'two') ) -# self.assertTrue( s.two ) self.assertTrue( s['two'] ) -# self.failIf( s.one ) self.failIf( s['one'] ) self.assertTrue( s.transition('two', 'three') ) From bf2bf29fc69fa82d8b0a882c99395ae25b11ab69 Mon Sep 17 00:00:00 2001 From: Thom Nichols Date: Tue, 8 Jun 2010 09:02:51 -0400 Subject: [PATCH 6/6] fixed mis-named variable, doc typo and using conformant Condition methods. --- sleekxmpp/xmlstream/statemachine.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sleekxmpp/xmlstream/statemachine.py b/sleekxmpp/xmlstream/statemachine.py index c6e0ea4..67b514a 100644 --- a/sleekxmpp/xmlstream/statemachine.py +++ b/sleekxmpp/xmlstream/statemachine.py @@ -97,7 +97,7 @@ class StateMachine(object): logging.debug(' ==== TRANSITION %s -> %s', self.__current_state, to_state) self.__current_state = to_state - self.lock.notifyAll() + self.lock.notify_all() return return_val # some 'true' value returned by func or True if func was None else: logging.error( "StateMachine bug!! The lock should ensure this doesn't happen!" ) @@ -126,13 +126,13 @@ class StateMachine(object): The other main difference between this method and `transition()` is that the - state machine is locked for the duration of the `with` statement (normally, - after a `transition() occurs, the state machine is immediately unlocked and + state machine is locked for the duration of the `with` statement. Normally, + after a `transition()` occurs, the state machine is immediately unlocked and available to another thread to call `transition()` again. ''' if not from_state in self.__states: - raise ValueError( "StateMachine does not contain from_state %s." % state ) + raise ValueError( "StateMachine does not contain from_state %s." % from_state ) if not to_state in self.__states: raise ValueError( "StateMachine does not contain to_state %s." % to_state ) @@ -228,7 +228,7 @@ class _StateCtx: self.state_machine.current_state(), self.to_state) self.state_machine._set_state( self.to_state ) - self.state_machine.lock.notifyAll() + self.state_machine.lock.notify_all() self.state_machine.lock.release() return False # re-raise any exception