Fix backend protocol version fallback bug

[glenebob]

Francisco,

Here is the fix for protocol version fallback.

The problem was that, after failing to startup on version 3, the NetworkStream.Dispose() was being called from .NET, which was closing the Connector even though it had abandoned that stream and started up with a new one.

-Glen

[franciscojunior]

I think this dispose bug is floating around for a long time.

We didn't get it before because it would only happen on protocol 2 only servers. Nice catch!

Some observations:

1. You forgot an "S" in the name of the NpgsqlConnector.BaseStream property. Currently it is Basetream :)
2. I think you could use the sslStram var directly instead of sslStreamPriv ?

One random comment:
I think in the future we would need to refactor NpgsqlNetworkStream.
Correct me if I'm wrong, but you had to create an exclusive field in NpgsqlConnector to hold it just to be able to access the AttachConnector method, right?

I think we could refactor NpgsqlNetworkStream to extend BufferedStream instead of NetworkStream (and maybe even change its name to just NpgsqlStream as it wouldn't be a networkstream anymore :) ) and use it as the NpgsqlConnector.Stream property, this way we would have a much better control about how to interact with the stream.
The idea of the NpgsqlNetworkStream, at the time of its creation, was only to fix a problem that when a .net domain is unloaded, all NpgsqlConnection objects were being closed and there was no way to send the terminate message to the server and postgresql would log a message about that. More information about that here: http://fxjr.blogspot.com.br/2011/12/fixed-log-unexpected-eof-on-client.html

But now I think we could use it to be a little smarter and encapsulate some of those stream logic and save us a lot of stream variables :) What do you think?

[glenebob]

On Mon, Oct 28, 2013 at 1:34 PM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

I think this dispose bug is floating around for a long time.

We didn't get it before because it would only happen on protocol 2 only
servers. Nice catch!

Some observations:

You forgot an "S" in the name of the NpgsqlConnector.BaseStream
property. Currently it is Basetream :)

Darn it! Sometimes I wish Visual Studio had a spell check, or better yet
a mind reading capability :)

I think you could use the sslStram var directly instead of
sslStreamPriv

The reason to use a different var is to eliminate type casting, mainly for
better readability.

One random comment:
I think in the future we would need to refactor NpgsqlNetworkStream.
Correct me if I'm wrong, but you had to create an exclusive field in
NpgsqlConnector to hold it just to be able to access the AttachConnector
method, right?

I think we could refactor NpgsqlNetworkStream to extend BufferedStream
instead of NetworkStream (and maybe even change its name to just
NpgsqlStream as it wouldn't be a networkstream anymore :) ) and use it as
the NpgsqlConnector.Stream property, this way we would have a much better
control about how to interact with the stream.
The idea of the NpgsqlNetworkStream, at the time of its creation, was only
to fix a problem that when a .net domain is unloaded, all NpgsqlConnection
objects were being closed and there was no way to send the terminate
message to the server and postgresql would log a message about that. More
information about that here:
http://fxjr.blogspot.com.br/2011/12/fixed-log-unexpected-eof-on-client.html

But now I think we could use it to be a little smarter and encapsulate
some of those stream logic and save us a lot of stream variables :) What do
you think?

Well, I certainly wasn't trying to solve every design issue related to this
:) I suspect the problem that was solved with the NpgsqlNetworkStream
class is that if the connection dies while the client is off doing
something else, there's no way for Npgsql to know about it and handle it
until the next time the client causes it to write to the stream again.

As we've discussed elsewhere, Npgsql should keep its own thread dedicated
to reading the network stream. Once we have that in place, this problem
solves itself, and I think we can actually eliminate NpgsqlNetworkStream
entirely, as problems on the socket would then be handled immediately when
Stream.ReadByte() throws an exception or returns -1, regardless of what the
client is doing, or indeed, even if the connector is lying dormant in the
connection pool.

So I think it's best to leave this alone as a known less-then-optimal
implementation until we can move forward with a comprehensive replacement.

-Glen

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/83#issuecomment-27253500
.

[franciscojunior]

It would be awesome if VS had a mind reading capability... :)

In fact, the problem NpgsqlNetworkStream solves isn't related to the connection being dropped. It was more related to the fact that when the client application exits and then terminate its process, Npgsql couldn't know, by itself, that it should close all the connections by sending the Terminate message to backend.

There is a problem though on Mono 3.x where this message still appears which makes me think that the dispose isn't being called. It used to be called on Mono 2.x though. I think there was some kind of regression.

I never checked why this problem appears on Mono 3.x because this error doesn't appear on ms.net and Mono 2.x so I really think this is a bug in Mono.

[glenebob]

Ah, I think I understand now.

Interesting. I added the finalizer to Connector, and there seems to be no
way to get it to be called before Dispose() on the network stream.

However. I re-wrote NpgsqlNetworkStream to simply not call base.Dispose(),
and then when Connector.~Connector() is called, it seems to be able to
successfully write the close message and close the connection. Dispose()
is still called first, but since it doesn't do anything, the stream is
still able to be shut down later via Close().

So maybe that's an approach to look at in the future, since it doesn't
require NpgsqlNetworkStream to know anything about its Connector object :)

-Glen

On Tue, Oct 29, 2013 at 9:44 AM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

It would be awesome if VS had a mind reading capability... :)

In fact, the problem NpgsqlNetworkStream solves isn't related to the
connection being dropped. It was more related to the fact that when the
client application exits and then terminate its process, Npgsql couldn't
know, by itself, that it should close all the connections by sending the
Terminate message to backend.

There is a problem though on Mono 3.x where this message still appears
which makes me think that the dispose isn't being called. It used to be
called on Mono 2.x though. I think there was some kind of regression.

I never checked why this problem appears on Mono 3.x because this error
doesn't appear on ms.net and Mono 2.x so I really think this is a bug in
Mono.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/83#issuecomment-27320236
.

[franciscojunior]

Yes, the destructors are the last ones to be called. The problem with destructors is that they are intended to release unmanaged resources only. And the stream is a managed resource. Although it relies on an unmanaged resource (socket). Also, in the destructor, .net doesn't guarantee the managed objects are available and haven't been disposed already.

That's why I implemented the npgsqlnetworkstream class and used its dispose method to close the connector.