feat: create Happy Eyeballs dialer

[fortuna]

No description provided.

[jyyi1]

Thanks! I have added some comments, also I would suggest to update the PR title to feat: add HappyEyeballsDialer

[jyyi1]

nit it seems this is the only place newClosedChan() is used. Maybe we can simplify close the channel here and deletenewClosedChan function to save some lines?

Suggested change

	var attemptDelayCh <-chan struct{} = newClosedChan()
	attemptDelayCh := make(chan struct{})
	close(attemptDelayCh)

[fortuna]

Your code doesn't work because attemptDelayCh must be of type <-chan struct{}, not chan struct{}, otherwise the assignment attemptDelayCh = waitCtx.Done() doesn't compile.

Instead of creating a channel, closing, and then assigning to attemptDelayCh, I found it better to keep that logic out of the core logic, which is already complicated.

[jyyi1]

SG, maybe var attemptDelayCh <-chan struct{} = make(chan struct{}) would work here?

[fortuna]

I tried that too 😅
The problem is that you can't close a read-only channel.

[jyyi1]

Can these lines be simplified with the following? Because we already have a case <-searchCtx.Done() statement in the select below, which will be triggered once searchCtx is cancelled. In addition, if we linked searchCtx to readyToDialCh, will there be any potential race conditions, because both case <-searchCtx.Done() and case <-readyToDialCh will be ready when searchCtx is cancelled?

Suggested change

	resolutionDelayCtx, cancelResolutionDelay := context.WithTimeout(searchCtx, 50*time.Millisecond)
	defer cancelResolutionDelay()
	readyToDialCh = resolutionDelayCtx.Done()
	readyToDialCh = time.Deplay(50*time.Millisecond)

[fortuna]

I'm assuming you meant time.NewTimer(50*time.Milisecond).C. I wasn't sure if we needed to drain the Timer.C channel, but it seems it's buffered and we don't need to?

In any case, I tried based on your suggestion, but the type of channel is different (<-chan time.Time), so it requires more changes, not really simpler. Also, the timer sends the time once, versus a context closes the channel, which is easier to reason about (a read will never block).

Another important point is that I still need to stop the timer, so I need to pass it to the goroutine to call Stop in case the dial returns before the timer triggers.

On the race condition, that's a valid point. A cancellation will potentially trigger an extra dial. I tried different ways to trigger, but couldn't. In any case, I changed the code to use the background context instead.

[fortuna]

Comments addressed

[fortuna]

Your code doesn't work because attemptDelayCh must be of type <-chan struct{}, not chan struct{}, otherwise the assignment attemptDelayCh = waitCtx.Done() doesn't compile.

Instead of creating a channel, closing, and then assigning to attemptDelayCh, I found it better to keep that logic out of the core logic, which is already complicated.

[fortuna]

I'm assuming you meant time.NewTimer(50*time.Milisecond).C. I wasn't sure if we needed to drain the Timer.C channel, but it seems it's buffered and we don't need to?

In any case, I tried based on your suggestion, but the type of channel is different (<-chan time.Time), so it requires more changes, not really simpler. Also, the timer sends the time once, versus a context closes the channel, which is easier to reason about (a read will never block).

Another important point is that I still need to stop the timer, so I need to pass it to the goroutine to call Stop in case the dial returns before the timer triggers.

On the race condition, that's a valid point. A cancellation will potentially trigger an extra dial. I tried different ways to trigger, but couldn't. In any case, I changed the code to use the background context instead.

[jyyi1]

The code LGTM, with some questions to be resolved. But the code flow seems a little bit complicated, hopefully we did not miss any race conditions or edge cases.

[jyyi1]

Here I mean we did not do anything special with dialDone in the last review. Why not just simply using the ctx parameter directly in the code below?

Suggested change

	// Indicates to attempts that the dialing process is done, so they don't get stuck.
	ctx, dialDone := context.WithCancel(ctx)
	defer dialDone()

[fortuna]

There's no guarantee that the input context will be cancelled. In this code, I cancel the context to make sure all subtasks are notifed and can complete.
If I remove the dialDone, the subtasks may hang for a while.

[jyyi1]

SG, maybe var attemptDelayCh <-chan struct{} = make(chan struct{}) would work here?

[jyyi1]

What's the benefit of using t.Run instead of defining a test function func TestIPv4HostShouldWork() {}?

[fortuna]

The subtests all get grouped nicely in the output and in the code, and I don't have to repeat the prefix all the time. Specially when I keep renaming things.

[jyyi1]

Suggested change

	type colletcStreamDialer struct {
	type collectStreamDialer struct {

[fortuna]

Fixed.

[fortuna]

I agree the flow is a bit complicated. I couldn't figure out a simpler way (it was a lot worse before). At least I have ~100% coverage for this code.

FYI, I generalized the API. Now you can customize the resolution phase to get v1 or v2 behavior. You can also inject hard-coded IPs, which will be quite useful.

I added documentation examples that are helpful illustrations.

[fortuna]

There's no guarantee that the input context will be cancelled. In this code, I cancel the context to make sure all subtasks are notifed and can complete.
If I remove the dialDone, the subtasks may hang for a while.

[fortuna]

I tried that too 😅
The problem is that you can't close a read-only channel.

[fortuna]

Fixed.

[fortuna]

The subtests all get grouped nicely in the output and in the code, and I don't have to repeat the prefix all the time. Specially when I keep renaming things.

[jyyi1]

Thanks for the contribution!