[Twisted-Python] Unruly Callback Code

[Tommi Virtanen]

On Tue, Aug 05, 2003 at 08:02:31AM -0600, Justin Johnson wrote:
> Would someone mind looking at the code below and suggesting ways to clean
> this up?

> 	# Subcommand: mkvob
> 	def cmd_mkvob(self):
> 		"""Usage: mkvob -v vob-tag[,vob-tag,...] -o site [-g group] [-s site,site,...]
> 		"""
> 		options = self.config.subOptions
> 		original_site = options["original"]
> 		group = options["group"]
> 		vobs = options["vobs"].split(",")
> 		sites = []
> 		if options["sites"] != None:
 		if options["sites"] is not None:
> 			sites = options["sites"].split(",")
> 
> 		def gotObject(object):
		def gotObject(object, vobs, group):
> 			self.perspectives[original_site] = object
> 			print "Successfully connected to %s (%s)." % (original_site, config.siteToServer[original_site])
> 			print "Calling remote mkvob ..."
> 			d = object.callRemote("mkvob", vobs, group)
> 			return d

	I'd actually avoid passing vobs and group into gotObject like
	that. Just have them as parameters; that means the function
	body is independent of its location, which I consider a lot
	cleaner.

> 		def renameReplica(results, vobs, original_site):
> 			# Do I have to connect again to do this?
> 			obj = self.perspectives[original_site]
> 			obj.callRemote("renameReplica", vobs, "original", original_site)
> 			return obj
> 
> 		def mkRepExport(results, vobs, sites, original_site):
> 			obj = self.perspectives[original_site]
> 			d = obj.callRemote("mkReplicaExport", vobs, sites)
> 			return d

	The tricks you play with self.perspectives are quite
	confusing, IMHO. I'd be more inclined to adding the
	callbacks accessing self.perspectives at a stage where
	I could pass them the perspective as an argument.
	That is,

		def gotObject(object, vobs, group):
			d = object.callRemote("mkvob", vobs, group)
			d.addCallback(renameReplica, object, vobs, original_site)
			d.addCallback(mkRepExport, object, vobs, sites, original_site)
			d.addCallback(mkRepImport, object, vobs, sites)
			return d

	Also, object is a bad name, as that name means other things in
	python2.2.

> 		def _stop(*args):
> 			print "The mkvob operation completed successfully"
> 			reactor.stop()
> 			return args

		def _stop(fail):
			reactor.stop()
			return fail

	That's not true, _stop is also called on errors.

	Also, _stop gets no extra arguments in .addBoth, so args=(aFailure,)
	and then you return that tuple.. 

> 		def mkRepImport(results, vobs, sites):
> 			if len(vobs) > 0 and len(sites) > 0:
> 				deferreds = []
> 				for site in sites:
> 					def onSuccess(object, site):
> 						self.perspectives[site] = object
> 						d = object.callRemote("mkReplicaImport", vobs)
> 						return d
> 					def applyTriggers(results, vobs, site):
> 						obj = self.perspectives[site]
> 						d = obj.callRemote("applyTriggers", vobs)
> 						return d
> 					d = self.connectToServer(config.siteToServer[site])
> 					d.addCallback(onSuccess, site).addErrback(log.err)
> 					d.addCallback(applyTriggers, vobs, site).addErrback(log.err)
> 					deferreds.append(d)
> 				return defer.DeferredList(deferreds)
> 			return results

	You just stepped in the DeferredList trap. DeferredList
	_never_ .errback()s unless it's told to fireOnOneErrback.
	That means you miss what errors might have happened.

	And fireOnOneErrback isn't normally what you want; you want
	to handle the results one by one.

> 		d = self.connectToServer(config.siteToServer[original_site])
> 		d.addCallbacks(gotObject, gotNoObject)
> 		d.addCallback(renameReplica, vobs, original_site).addErrback(log.err)
> 		d.addCallback(mkRepExport, vobs, sites, original_site).addErrback(log.err)
> 		d.addCallback(mkRepImport, vobs, sites).addErrback(log.err)

	There's no need to .addErrback that much. There's probably no
	reason to have gotNoObject special cased, and handle later
	errors just with log.err.

		d = self.connectToServer(config.siteToServer[original_site])
		d.addCallback(gotObject, vobs, group)
		d.addCallback(renameReplica, vobs, original_site)
		d.addCallback(mkRepExport, vobs, sites, original_site)
		d.addCallback(mkRepImport, vobs, sites)
		d.addErrback(log.err)

-- 
:(){ :|:&};: