Making LookupClient disposable and improved TCP client pool#220
Making LookupClient disposable and improved TCP client pool#220
Conversation
Making LookupClient disposable. Disposing tcp client pool properly.
antonfirsov
left a comment
There was a problem hiding this comment.
A couple of remarks. Since this is a significant breaking change, I would recommend to signal it to the package consumers by a major version bump.
| } | ||
| catch (Exception) when (gotCanceled) | ||
| { | ||
| throw new TimeoutException("Connection timed out."); |
There was a problem hiding this comment.
The caller Query method doesn't seem to translate cancellations to TimeoutException.
There was a problem hiding this comment.
Not exactly sure what you mean by that.
What I want to do here is allowing LookupClient does handle OperationCanceled- and Timeout-Exceptions and retry the request if possible.
For TCP, if the connection attempt takes longer then the configured timeout for a query, I want to handle it the same way and maybe have LookupClient retry.
There was a problem hiding this comment.
changed both, sync and async version to throw new OperationCanceledException("Connection timed out.", cancellationToken);
Still, its mostly a signal for LookupClient to retry
| } | ||
| catch (Exception) when (gotCanceled) | ||
| { | ||
| throw new TimeoutException("Connection timed out."); |
There was a problem hiding this comment.
Since this method is async, it shouldn't be translated to TimeoutException.
There was a problem hiding this comment.
I'll change both, the sync and async version to throw new OperationCanceledException("Connection timed out.", cancellationToken);
Making LookupClient and DnsMessageHandlers disposable.
Primarily to dispose the TcpClient-pool.
The pool could be empty if the TCP fallback was either disabled or never used, so it is not always necessary to dispose the LookupClient owning that resource.