-
Notifications
You must be signed in to change notification settings - Fork 824
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
Read with timeout is incompatible with SslStream #1501
Comments
@vonzshik since you're working with timeouts, you cant take a look at this. Otherwise, I could take this a bit later since already did some SSL stuff. |
@YohDeadfall sure I can take a look - but doesn't it mean, that ReadBuffer has the same problem? |
Yes, it has. The buffer is just a wrapper around the stream. |
Interestingly enough, it seems to work fine for .net 5, but not for .net 4.6.1 |
Oh, very good! If it also works for .NET 3.1 we could just close this issue because .NET Framework is on the way to a graveyard. |
You mean that SslStream actually works well with timeouts now? BTW I assume we're discussing sync timeouts, I wonder if async ones (via cancellation tokens) work? Related (but the outcome of that issue isn't clear): dotnet/runtime#22496 |
I think we can just conditional-compile the check to only throw for TFMs where the timeout doesn't work (i.e. net461). |
I would also assume that it doesn't work on .NET Core 3.1 because of dotnet/runtime#22496 |
It actually does work - results. [Test]
public void WaitWithSsl()
{
var csb = new NpgsqlConnectionStringBuilder(ConnectionString)
{
SslMode = SslMode.Require,
TrustServerCertificate = true,
};
using (var conn = OpenConnection(csb.ToString()))
{
conn.ExecuteNonQuery("LISTEN notifytest");
Assert.That(conn.Wait(1000), Is.EqualTo(false));
Assert.That(conn.ExecuteScalar("SELECT 1"), Is.EqualTo(1));
}
} |
Well then... great! So I guess just conditionally-compile the check for net461? |
(Technically there's also 2.x, with 2.1 being still supported. We can check what's going on there if we want to be complete) |
Nope for async ones, as .net 4.6.1 doesn't support the cancellation on the socket level through the cancellation token. |
Right, but maybe SslStream does support cancellation tokens in .NET 5.0, just like they improved NetworkStream to do it? |
I'm not sure how it would help. The problem is, |
NetworkStream definitely does honor cancellation tokens (see dotnet/corefx#36516), otherwise this whole work we're doing would have been for nothing :) SslStream also seems to flow cancellation tokens, and so maybe honors them too (see ReadAsync, and then this. We can test this to be sure. |
@roji I'm talking about .net 4.6.1, not .net core. AFAIK, it still uses the same libraries from .net 4.6.1. |
And that is why every test with the cancellation token is disabled for .net 4.6.1 - they would be stuck forever. |
Also, wouldn't it in case of ,net standard 2.0 and 2.1 be entirely dependent on runtime? |
Ah OK, I misunderstood.
For sync Wait, we've been throwing up to now if a timeout is specified but the connection is secure. I was proposing that if we keep that behavior but only for .NET 4.6.1, the test could test it (i.e. that we throw in those conditions). In other words, instead of not have the test at all for NET461, we could have a version that asserts that we throw the right exception. If we do this, I think it makes sense to also do the same for async.
Good point - we indeed shouldn't go into differentiating .NET Framework from .NET Core at runtime. |
And I've also misunderstood 😄. |
I agree. We can just let it do the timeout on netstandard2.0 (i.e. not block it), and if the user is on .NET Framework it's their problem... |
This sounds like a problem that will slowly fade away on its own. We also haven't gotten many new issues around this, closing. |
As @Emill wrote in gitter, our current synchronous implementation of
Wait()
will fail if a timeout is passed and SslStream is used, because:See http://stackoverflow.com/questions/37177401/how-to-repeatedly-read-from-net-sslstream-with-a-timeout.
Note sure if there's a good workaround for SslStream. For now, adding a check that throws NotSupportedException.
The text was updated successfully, but these errors were encountered: