Conversation
… Dns.GetHostAddressesAsync
| } | ||
| catch (OperationCanceledException oce) when ( | ||
| oce.CancellationToken == combinedToken && !cancellationToken.IsCancellationRequested) | ||
| catch (OperationCanceledException) |
There was a problem hiding this comment.
I'm wondering if there's value in bubbling up the specific OperationCanceledException thrown by Dns.GetHostAddressesAsync, rather than a new one generated by ThrowIfCancellationRequested below (I think that was probably the thinking behind the current logic...)... I'm not sure there is.
But to keep the behavior that way, it may be a bit better to just keep the !cancellationToken.IsCancellationRequested in the when for the timeout (or am I misunderstanding the issue)?
In any case, I'm also OK with the change as-is and will approve.
There was a problem hiding this comment.
The issue is that OperationCancelledException has a property CancellationToken. And because we use CancellationTokenSource the token there will not be the one user passed to NpgsqlConnection.OpenAsync, but from that CancellationTokenSource. Most of the time no one really cares what token it has, but in our case we ourselves actually do care:
npgsql/src/Npgsql/NpgsqlMultiHostDataSource.cs
Lines 221 to 226 in f9dd919
There was a problem hiding this comment.
OK. Am on vacation and don't have time to review more deeply, and in any case the change looks OK, just a bit odd. No problem for merging.
|
Backported to 10.0.1 via aa0e138 |
Fixes #6362