Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SyncDNS connection string parameter #382

Closed
roji opened this issue Oct 13, 2014 · 15 comments
Closed

Add SyncDNS connection string parameter #382

roji opened this issue Oct 13, 2014 · 15 comments

Comments

@roji
Copy link
Member

roji commented Oct 13, 2014

Our current connection mechanism resolves DNS with an asynchronous call; this is because the .NET sync DNS API provides no timeout facility, and we're bound by ADO.NET to provide a connection timeout.

We've had several reports of people having trouble with this mechanism, in cases of bursts: the threadpool is exhausted, the DNS async callback is delayed, up to a timeout. See #376 and this extensive discussion.

Assuming no alternative DNS client implementation supporting a timeout can be found, we should provide an option to the user to select the .NET sync DNS, which would mean giving up the timeout for a completely synchronous connection process (which doesn't depend on the threadpool).

@roji roji added this to the 2.2.2 milestone Oct 13, 2014
@franciscojunior
Copy link
Member

I think this project could be used: DNS.NET Resolver .
http://www.codeproject.com/Articles/23673/DNS-NET-Resolver-C

The author says it supports timeout and async operations. Also, code license seems to be compatible with Npgsql.

@Emill
Copy link
Contributor

Emill commented Oct 13, 2014

I think this is a good idea. DNS results are probably cached anyway by the OS. Otherwise, the OS has an own timeout mechanism for DNS queries (I think), which will then throw a SocketException, which means we never get infinite timeouts.

.NETs async DNS isn't really i/o async; it simply offloads a sync DNS query on a worker thread.
I checked how MySQL does this. They do sync DNS with no timeout. Connection timeout is only used on establishing the TCP connection and sending init packets.

I think this project could be used: DNS.NET Resolver .
http://www.codeproject.com/Articles/23673/DNS-NET-Resolver-C

The author says it supports timeout and async operations. Also, code license seems to be compatible with Npgsql.

I looked at that too. It seems interesting. Async is implemented by offloading the work to a worker thread. But it should be possible to modify it (since it is a pretty basic Udp query) to be fully async. We must, though, make sure it respects the machine's hosts file and allow things like "localhost".

@roji
Copy link
Member Author

roji commented Oct 13, 2014

I think I saw this too at some point. It's really tempting to fix the problem like this, but I'm a bit worried about having our own in-house DNS implementation inside Npgsql.dll... As @Emill says, the machine host file and DNS config file (e.g. Linux /etc/resolv.conf) needs to be supported but also other resolution methods (in theory NIS) - this package appears to only do the pure DNS client part. At least with the .NET DNS it's all managed, and we can be sure it will always work correctly...

@Emill, it's interesting that MySQL does simple sync DNS with no timeout. I'm still convinced that the whole timeout approach is not ideal (see discussion), but ADO.NET does require us to do something. Do you guys want to try and look into DNS.NET?

@franciscojunior
Copy link
Member

I checked how MySQL does this. They do sync DNS with no timeout. Connection timeout is only used on establishing the TCP connection and sending init packets.

I thought about that idea too. That we counted the timeout only for the server connection establishment not including the dns resolve time. But then I thought that if the dns takes too much time to resolve, the timeout specified by the user wouldn't be used.

@franciscojunior
Copy link
Member

I think I saw this too at some point. It's really tempting to fix the problem like this, but I'm a bit worried about having our own in-house DNS implementation inside Npgsql.dll... As @Emill says, the machine host file and DNS config file (e.g. Linux /etc/resolv.conf) needs to be supported but also other resolution methods (in theory NIS) - this package appears to only do the pure DNS client part. At least with the .NET DNS it's all managed, and we can be sure it will always work correctly...

That's true. @Emill points are totally valid. I didn't check this support though.

@franciscojunior
Copy link
Member

@Emill, it's interested that MySQL does simple sync DNS with no timeout. I'm still convinced that the whole timeout approach is not ideal (see discussion), but ADO.NET does require us to do something. Do you guys want to try and look into DNS.NET?

I wonder what sqlclient does. Maybe not even sqlclient handles dns resolution timeout. :)
This would be nice as we could make just like MySQL.

@Emill
Copy link
Contributor

Emill commented Oct 13, 2014

Too bad the connection mechanism in SqlClient is closed source (they use native code instead of managed code), so it's not possible to see how they do it. However, the mono implementation at https://github.com/mono/mono/blob/master/mcs/class/Mono.Data.Tds/Mono.Data.Tds.Protocol/TdsComm.cs#L105 uses sync dns resolve with no timeout.

@Emill
Copy link
Contributor

Emill commented Oct 13, 2014

FYI: postgresql jdbc doesn't seem to use any timeout on dns lookup either; only on the connection attempt itself.

@roji
Copy link
Member Author

roji commented Oct 13, 2014

@Emill, any chance you can post a summary of all your findings and arguments to the Google Groups dev discussion? I'm not sure Glen and Josh are reading these github threads

@jbcooley
Copy link
Contributor

I'm following them. It's just hard to keep up and comment on the discussion.

@glenebob
Copy link
Contributor

I'm following as much as I can, too, just nothing to add. ( just don't have the time to dig in and try to find and prove a better solution.

-Glen

@roji
Copy link
Member Author

roji commented Oct 14, 2014

Sorry guys, didn't meant to imply anything, just wanted to make sure you were in the loop.

@roji roji modified the milestones: 2.2.3, 2.2.2 Nov 1, 2014
@roji roji modified the milestones: 2.2.3, 2.2.4 Nov 26, 2014
@roji roji modified the milestone: 2.2.4 Feb 5, 2015
@roji roji added this to the 3.1 milestone Feb 14, 2015
@roji
Copy link
Member Author

roji commented Apr 26, 2015

Revisiting this issue after so much time... once we finish implementing OpenAsync(), I think the Open() path should be sync-only (no reliance on any external thread), while OpenAsync() should be async-only. This way, if someone really cares about DNS lookup timeouts (which we can't do synchronously) they can simply use OpenAsync().

A truly synchronous timeout on the socket connection (as opposed to the DNS lookup) can be performed by setting the socket to non-blocking mode, doing Connect() and then Select().

Once the above two are implemented, synchronous Npgsql connections will no longer be affected by thread pool starvation, etc.

@roji roji closed this as completed Aug 7, 2015
@arathorn
Copy link

arathorn commented Aug 8, 2015

Was this issue resolve in the 3.0.0 release?

@roji
Copy link
Member Author

roji commented Aug 8, 2015

No, 3.0's connection process still involves asynchronous DNS.

However, for 3.1 I plan to make Open() entirely synchronous, with OpenAsync() being an entirely asynchronous alternative (see last comment).

@roji roji removed this from the 3.1 milestone May 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants