nictuku / dht

Kademlia/Mainline DHT node in Go.
Other
826 stars 144 forks source link

wrapper around Run() ? #44

Closed rkjdid closed 9 years ago

rkjdid commented 9 years ago

I think there's a small issue regarding the dht.Run() function, it both contains an initialization phase, prone to error (listen fails), and the main loop which I think suggests that it should be run in a go routine, since the only other way out is dht.Stop() from another routine.

It makes the handling of the first error a bit tricky. It also leads to another trickiness : a call to dht.Port() method is prone to hanging if listen fails and we did not check Run()'s return value - at least a close(dht.portRequest) would be necessary I think

I'm not sure about the best way to handle this, or if you wish to change anything at all, ideally I think we should accept the main loop will be running in a routine, and go dht.loop() ourselves in there as a private function if initialization (listen) was successful - loop not giving any error. It would allow the main entry point to return everytime, so we know the dht is running, or not. A downside to this is that it will induce retro-compatibilities issues, since we cannot expect dht.Run() to block - maybe something like an alternate dht.Start(), with depreciation on Run ?

nictuku commented 9 years ago

I am happy to make changes if they lead to a more robust library. This code was written a long time ago and I'm certain it could be improved - and I'm happy to merge changes for that.

If you could make a PR with your proposal, it's easier to review and comment.

Overall I'm OK with the idea of introducing a new method (dht.Start) that is more robust, and keeping the old one around for a few months.

But could you explain why func dht.Start() error is different from func dht.Run() error ? Specifically, you said "It makes the handling of the first error a bit tricky". Can you explain?

rkjdid commented 9 years ago

Well, there are 2 distinct behaviours in the function, either it exits with an error code, or it doesn't exit at all until dht.Close() is actively called. A common usage with the current code would be go dht.Run() (this is what's done in taipei for example) if we wish to continue working around the dht, which disregards the error. Simply calling err = dht.Run() and checking the error wouldn't work because on success the caller would just be hanging. To work around this we would need check the error in the routine and communicate somehow that the process is correctly running/hanging in/on Run(), this is what I think is unnatural.

The idea is to split these 2 phases, the initialization that can fail, and the for {} loop, so the entry point is guaranteed to return & tell the caller if the dht was correctly started or not - anyway thank's for the good will and quick answers, let me do a PR we'll discuss from there

nictuku commented 9 years ago

Got it. Let's do it. Want to send a PR?

rkjdid commented 9 years ago

incoming, I'm having weirdnesses with the git tree

nictuku commented 9 years ago

It's OK. I may not be able to merge it right away, either.