The context here is an application that publishes an onion service, but the user closes the app while everything is still being initialized. I want to close things down as gracefully as possible in that situation.
If tor.Start has already returned and we've got a reference to a *Tor we can Close() it, but if t.Listen has not returned yet, we don't have an *OnionService we can Close() first, even though we're in the process of publishing one. So we try to t.Close() while the onion service is still being published and occasionally we see:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x7ae17e]
goroutine 8 [running]:
github.com/cretz/bine/control.(*Conn).debugEnabled(...)
/home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/conn.go:91
github.com/cretz/bine/control.(*Conn).SendRequest(0x0, {0x1018d1c, 0xc00038b6c0}, {0xc00038b718, 0xc00038b728, 0x1})
/home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/conn.go:56 +0x5e
github.com/cretz/bine/control.(*Conn).sendRequestIgnoreResponse(...)
/home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/conn.go:47
github.com/cretz/bine/control.(*Conn).DelOnion(...)
/home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/cmd_onion.go:200
github.com/cretz/bine/tor.(*OnionService).Close(0xc00019e310)
/home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/tor/listen.go:325 +0x13b
github.com/cretz/bine/tor.(*Tor).Listen(0xc0003c6000, {0x114e930, 0xc00013c000}, 0xc00038bb78)
/home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/tor/listen.go:291 +0x141e
...
I tried using a cancelable context passed into t.Listen, and calling the cancel function before closing tor, but that hung for reasons unknown.
I think what's going on here is that (*Tor).Close assigns the control socket to nil, then somewhere in (*Tor).Listen there's an error, which at the end, results in an attempt to close the onion service. But by this point tor is closed and the control port is nil, so the onion service's closer is probably hitting the nil pointer dereference here. I thought about just PRing a nil check there, however I noticed there were other calls in the (*Tor).Listen function that might have issues.
So maybe the actual solution here is just to not assign the control port to nil, and just let all the other calls check and early return on "use of closed connection" errors. That's probably the most concurrency safe option, but I wanted to discuss it a little before submitting a PR. Alternatively I suppose the cancel function should really be what's used, and perhaps that's what I need to fix (potentially user error, I didn't spend to long on it) and a panic when t.Listen isn't canceled properly is expected.
Let me know what you make of this and if there's anything you'd like me to submit.
The context here is an application that publishes an onion service, but the user closes the app while everything is still being initialized. I want to close things down as gracefully as possible in that situation.
If
tor.Starthas already returned and we've got a reference to a*Torwe canClose()it, but ift.Listenhas not returned yet, we don't have an*OnionServicewe canClose()first, even though we're in the process of publishing one. So we try tot.Close()while the onion service is still being published and occasionally we see:I tried using a cancelable context passed into
t.Listen, and calling the cancel function before closing tor, but that hung for reasons unknown.I think what's going on here is that
(*Tor).Closeassigns the control socket to nil, then somewhere in(*Tor).Listenthere's an error, which at the end, results in an attempt to close the onion service. But by this point tor is closed and the control port is nil, so the onion service's closer is probably hitting the nil pointer dereference here. I thought about just PRing a nil check there, however I noticed there were other calls in the(*Tor).Listenfunction that might have issues.So maybe the actual solution here is just to not assign the control port to nil, and just let all the other calls check and early return on "use of closed connection" errors. That's probably the most concurrency safe option, but I wanted to discuss it a little before submitting a PR. Alternatively I suppose the cancel function should really be what's used, and perhaps that's what I need to fix (potentially user error, I didn't spend to long on it) and a panic when
t.Listenisn't canceled properly is expected.Let me know what you make of this and if there's anything you'd like me to submit.