Improve error messages when remote addresses are bad

[anacrolix]

Improves the error message when there are no addresses because they're all filtered out, or none are given. Also the dial logic is simplified. Timing, and pseudo-random channel operations make optimistic selects unworthwhile.

[anacrolix]

Fixes #69.

[raulk]

I do not think this PR brings any improvement, and in fact it adds a superfluous goroutine and makes the code less readable by substituting a digestible loop with a dance of boolean flags, counters and unnecessary synchronisation because the whole thing has no reason to be multithreaded.

The original intention was based on wrong assumptions, that we clarified during conversation.

The title is also misleading, as it suggests we're only improving an error code, but in reality it attempts to sneak in a refactor of the dialling loop.

@anacrolix I think we're better off closing this PR – I'll let you do it. Please submit the error message improvement separately.

[anacrolix]

I've refactored this back to the channel model, with the optimistic select. I need the more specific error messages as "default failure" is getting really old really quick and making debugging a pain.

[raulk]

@anacrolix I don't like this mixture of functional and procedural (and the added indirection) just to shortcut 2-3 lines of code. To me, the value prop is not there, especially given that repetition in Go is idiomatic. I much rather read this linearly than seesawing up and down.

[raulk]

I suggest this PR sticks to introducing an exported error for when we have no remote addresses to dial.

[anacrolix]

I'm on board with that error now, I'll add it.

[anacrolix]

@raulk This error, if exposed, belongs in go-libp2p-net, as a part of the Dialer and Network interfaces there.

[anacrolix]

@anacrolix I don't like this mixture of functional and procedural (and the added indirection) just to shortcut 2-3 lines of code. To me, the value prop is not there, especially given that repetition in Go is idiomatic. I much rather read this linearly than seesawing up and down.

It's explicit about identical behaviour on different sets of communication clauses in the select. I think it's worth doing, especially with the extensive behaviour and comments in onDialResult. ctxDoneRetErr is there for symmetry. I don't want to bike-shed this part.

[anacrolix]

This now requires libp2p/go-libp2p-net#38 to be merged.

[raulk]

@anacrolix

I don't want to bike-shed this part.

Isn't 90% of this PR a bike-shed of master?

First, would you mind making this PR stick to its title?

Second, you're proposing to introduce a personal stylistic preference on code that has been vetted by the community at large. As I explained above, IMO your change introduces extra bookkeeping variables, and makes it unreadable linearly. This is a critical part of the system where explicitness is appreciated. Optimistic selects are used largely in the codebase in their denormalised form. I also believe that, in Go, repetitiveness is OK and idiomatic. Moreover, the resulting block has no substantial reduction in LoC either, so by all accounts I really don't understand your insistence here.

To move forward, I propose to stop beating a dead horse. Can you close this PR and open a new one, just with the changes announced in the title, which are pretty non-controversial?

If you really want to push the closure stuff, open a separate PR just for that and try to get others to chime in; you already know where I stand ;-)