bine icon indicating copy to clipboard operation
bine copied to clipboard

Embedded start/check data race

Open karalabe opened this issue 7 years ago • 3 comments

If I start an embedded tor process, you are currently starting it and immediately checking control port status https://github.com/cretz/bine/blob/master/tor/tor.go#L142:

	err := tor.startProcess(ctx, conf)
	// Connect the controller
	if err == nil {
		err = tor.connectController(ctx, conf)
	}

this however is racey, because it's almost impossible for tor to start up fast enough for the control port to be open by the time you reach the check. If I explicitly add a couple second sleep before connect, everything works nicely, otherwise starting just fails with Error on start: dial tcp 127.0.0.1:9051: connect: connection refused.

karalabe avatar Jun 29 '18 10:06 karalabe

Btw, I played around with this code a bit and your code seems to work fine with Tor 0.3.3, but the racey behavior appears with Tor 0.3.5. Probably they changed initialization order a bit so the new Tor gets to the control port later than the current stable one.

karalabe avatar Jun 29 '18 16:06 karalabe

True, suggestions? I am not sure the best cross-platform way to wait until a port is open by an external process before proceeding.

I can update connectController to leverage the context's doneness (I should do this anyways with net.DialContext+textproto.NewConn since textproto doesn't have its own DialContext) and keep retrying. Unfortunately, for this to be reasonable I might need to only retry on connection refused instead of other connection failure that would force the caller to wait until timeout.

I can also wait on some stdout output from the tor process I suppose, but I want the user to be able to control output verbosity.

I could also do this in embeddedProcess.start to have it sleep for a (configurable?) sec.

Suggestions?

cretz avatar Jun 29 '18 16:06 cretz

Ah, I bet this works with an 0 for control port because it waits for the control port file to be written. Maybe I should always use a control port file by default, expose a NoControlPortWriteFile bool, and apply the current auto-port-lookup-via-file logic to all situations?

cretz avatar Jun 29 '18 16:06 cretz