hs-connection icon indicating copy to clipboard operation
hs-connection copied to clipboard

Asynchronous search for successful IP

Open lolepezy opened this issue 3 years ago • 4 comments

This is to introduce a minimal (and not complete) implementation of RFC 6555 and RFC 8305.

Instead of doing manual asynchronous DNS lookup for AAAA and A records, it relies on getAddrInfo.
Available IP addresses are chosen concurrently.

The reason for the change is to avoid long timeouts when a domain has AAAA DNS record but no IPv6 connectivity, which is a relatively common misconfiguration.

lolepezy avatar Dec 20 '21 09:12 lolepezy

I generally like the idea a lot. A few nitpicks:

  • this keeps the bug described in #51
  • forConcurrently will not cancel the other attempts, they may keep running in the background, should we use withAnyCancel?
  • even if we use withAnyCancel I'm not 100% convinced we won't end up with 2 or more open connections in some edge cases - or am I missing something? Think one action completes just as another one has opened the connection and is about to tryPutMVar. This may be tricky to get right.

Provided those are fixed I'd be in favor of merging this over #52.

mulderr avatar Jul 05 '22 20:07 mulderr

I generally like the idea a lot. A few nitpicks:

* this keeps the bug described in [HostNotResolved can never happen? #51](https://github.com/vincenthz/hs-connection/issues/51)

I rebased the change on top of mulderr:master.

* `forConcurrently` will not cancel the other attempts, they may keep running in the background, should we use `withAnyCancel`?

I expected forConcurrently to throw AsyncCancelled to all the threads, am I not right about it? I also tried an implementation based on recursion with withAsync so we only create as many asyncs as needed, but it makes code a little more complicated. I can push such a change, would make sense to try.

* even if we use `withAnyCancel` I'm not 100% convinced we won't end up with 2 or more open connections in some edge cases - or am I missing something? Think one action completes just as another one has opened the connection and is about to `tryPutMVar`. This may be tricky to get right.

Oh, that's a good point! I pushed a commit where tryPutMVar is under the same mask called by bracketOnError, so the behavior should be atomic. But I am not sure now if it's problematic (can there be a deadlock?) to have a masked tryPutMVar, hopefully not.

lolepezy avatar Jul 24 '22 11:07 lolepezy

it's looking interesting, but honestly no time to review / look in details on what's happenning. anyone interested in taking over or just having writable permission to this repo ?

vincenthz avatar Aug 19 '22 04:08 vincenthz

I would be happy to help or even take over if no one else comes forward.

--

I ran some simple tests. LGTM. Also I see http-client is already using an almost identical implementation.

It would be ideal to have Connection Attempt Delay be configurable through ConnectionParams but that would cause quite a lot of turbulence downstream so I'd put that off for a possible major version down the line.

mulderr avatar Aug 24 '22 17:08 mulderr