System.InvalidOperationException in FirebirdSql.Data.FirebirdClient about dapper

[mgravell]

Interesting; I have a repro; this is actually caused by the disposal of the DbCommand object inside ExecuteReaderImpl. I am tempted to say that this is actually a bug in the firebird ADO.NET implementation - disposing the command does not usually kill the reader. I'm going to float this past the provider authors, to see what they think.

[mgravell]

I have submitted a pull request to the provider (https://sourceforge.net/p/firebird/NETProvider/merge-requests/2/); there isn't much more I can do... I'm reluctant to not dispose the commands. Thoughts?

[guntama]

Thank you for dealing with this matter.
I agree with you . I also think this is a problem in FirebirdClient.

[mgravell]

cheers; I can't force firebird to take the PR, but I've done what I can

[cincuranet]

Hi Marc. I'm the one responsible for FirebirdClient. Thanks for the patch (BTW there's a mirror on GitHub, which is more pleasant to work with, IMO), sadly it's wrong.

First the Dispose method itself also calls this.activeReader = null; and this.statement = null;, so just skipping closing reader in Release is not enough.

Unfortunately as the code is now the reader itself needs command to fetch records. Initially only few are fetched (whatever fits the first response buffer) and then more is fetched as the buffer is processed. The command (actually something called "statement") holds the resources on server and handles all that and command keeps all the references etc. I know it's not a best design, but that's how it started and is now. :(

I wish to have some quick fix, but probably only a big rewrite would work. Could you not dispose the command before reader on your side?

[mgravell]

Thanks for the reply. If the nulls were the only issue, it could of course
be tweaked to set those to null.

Re the command vs reader: it kinds sounds like at this point ownership has
passed to the reader, and thus the correct place to manage that is in the
reader, not the command. I fully sympathise, however, as one library author
to another, that this type of change is not trivial. Ultimately, the vexing
thing is that it works fine for every other ADO.NET provider.

Some of the APIs : yes, I could change the implementation to keep the
command action for longer. Not convenient, but possible. However, there is
also an API on dapper that returns the reader to the caller without ever
showing them the command. If I don't dispose the command, no-one ever will
(no-one else ever sees it). I can't wrap the reader, as the caller might
be casting to the provider-specific version for: reasons.

I guess I can patch up dapper to get by as best it can, but long term, I do
still think this is something that belongs in the reader...

Marc
On 6 Oct 2014 13:20, "Jiri {x2} Cincura" [email protected] wrote:

Hi Marc. I'm the one responsible for FirebirdClient. Thanks for the patch
(BTW there's a mirror on GitHub
https://github.com/cincuranet/NETProvider, which is more pleasant to
work with, IMO), sadly it's wrong.

First the Dispose method itself also calls this.activeReader = null; and this.statement
= null;, so just skipping closing reader in Release is not enough.

Unfortunately as the code is now the reader itself needs command to fetch
records. Initially only few are fetched (whatever fits the first response
buffer) and then more is fetched as the buffer is processed. The command
(actually something called "statement") holds the resources on server and
handles all that and command keeps all the references etc. I know it's not
a best design, but that's how it started and is now. :(

I wish to have some quick fix, but probably only a big rewrite would work.
Could you not dispose the command before reader on your side?

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

[cincuranet]

Re the command vs reader: it kinds sounds like at this point ownership has
passed to the reader, and thus the correct place to manage that is in the
reader, not the command.

I agree. You can create (or I'll later) a ticket at tracker.firebirdsql.org. There's a couple of bad pieces of code there. I'm slowly trying to remove them.

I can't wrap the reader, as the caller might
be casting to the provider-specific version for: reasons.

I see. Probably only overriding casting operator would work. But that's hassle.

[mgravell]

Overriding the casting simply operator isn't viable:

• the return type is IDataReader; you can't add a custom casting operator
  to an arbitrary interface
• the code that would need to do this doesn't know the type of reader, so
  can't expose a custom conversion operator to that type - the reader comes
  from ADO.NET; dapper does not include assembly references for firebird,
  oracle, mysql, etc...

On 7 October 2014 05:49, Jiri {x2} Cincura [email protected] wrote:

Re the command vs reader: it kinds sounds like at this point ownership has
passed to the reader, and thus the correct place to manage that is in the
reader, not the command.

I agree. You can create (or I'll later) a ticket at
tracker.firebirdsql.org. There's a couple of bad pieces of code there.
I'm slowly trying to remove them.

I can't wrap the reader, as the caller might
be casting to the provider-specific version for: reasons.

I see. Probably only overriding casting operator would work. But that's
hassle.

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

Regards,

Marc

[cincuranet]

OK, well. I think currently the FbDataReader is out of business. Either I'll rework it or maybe somebody will contribute.

[mgravell]

"out of business"?

On 7 October 2014 08:34, Jiri {x2} Cincura [email protected] wrote:

OK, well. I think currently the FbDataReader is out of business. Either
I'll rework it or maybe somebody will contribute.

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

Regards,

Marc

[cincuranet]

Like that it will not work with Dapper correctly.

[mgravell]

Since dapper is just using ADO.NET here, it could be that it fails in a
number of other tools that use ADO.NET; but... it is what it is.

On 7 October 2014 09:23, Jiri {x2} Cincura [email protected] wrote:

Like that it will not work with Dapper correctly.

—
Reply to this email directly or view it on GitHub
#178 (comment)
.

Regards,

Marc

[mgravell]

This has been "fixed" (worked around) in the most recent dapper commit - which wraps the reader such that the command's lifetime is linked to the reader. Not ideal, but it works.