Skip to content

System.InvalidOperationException in FirebirdSql.Data.FirebirdClient #178

Closed

Description

@guntama

This code succeeds in raw ADO.net Executereader(), but fails in dapper Executereader().
IDataReader.Read() throws a System.InvalidOperationException exception with the message:

Invalid attempt of read when the reader is closed.
using System;
using System.Data;
using Dapper;
using FirebirdSql.Data.FirebirdClient;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            var cs = @"initial catalog=localhost:database;user id=SYSDBA;password=masterkey";
            var sql = @"select count(*) from user";
            var con = new FbConnection(cs);
            con.Open();

            // raw ADO.net
            var sqlCmd = new FbCommand(sql, con);
            IDataReader reader1 = sqlCmd.ExecuteReader();
            while (reader1.Read())
            {
                Console.WriteLine(reader1[0].ToString()); // output is 10
            }

            // dapper
            var reader2 = con.ExecuteReader(sql);
            while (reader2.Read()) // System.InvalidOperationException. "Invalid attempt of read when the reader is closed."
            {
                Console.WriteLine(reader2[0].ToString()); 
            }
            Console.Read();
        }
    }
}

Stack trace:

System.InvalidOperationException was unhandled by user code.
  HResult=-2146233079
  Message=Invalid attempt of read when the reader is closed.
  Source=FirebirdSql.Data.FirebirdClient
  StackTrace:
       at FirebirdSql.Data.FirebirdClient.FbDataReader.CheckState()
       at FirebirdSql.Data.FirebirdClient.FbDataReader.Read()
       at ConsoleApplication2.Program.Main(String[] args) at c:\Users\aoki\Desktop\ConsoleApplication2\ConsoleApplication2\Program.cs:行 33
       at System.AppDomain._nExecuteAssembly(RuntimeAssembly assembly, String[] args)
       at System.AppDomain.ExecuteAssembly(String assemblyFile, Evidence assemblySecurity, String[] args)
       at Microsoft.VisualStudio.HostingProcess.HostProc.RunUsersAssembly()
       at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
       at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
       at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
       at System.Threading.ThreadHelper.ThreadStart()
  InnerException: 

Activity

  1. mgravell commented on Sep 22, 2014

    @mgravell
    Member

    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.

  2. mgravell commented on Sep 22, 2014

    @mgravell
    Member

    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?

  3. guntama commented on Sep 22, 2014

    @guntama
    Author

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

  4. mgravell commented on Sep 22, 2014

    @mgravell
    Member

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

  5. cincuranet commented on Oct 6, 2014

    @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?

  6. mgravell commented on Oct 6, 2014

    @mgravell
    Member

    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)
    .

  7. cincuranet commented on Oct 7, 2014

    @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.

  8. mgravell commented on Oct 7, 2014

    @mgravell
    Member

    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

  9. cincuranet commented on Oct 7, 2014

    @cincuranet

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

  10. mgravell commented on Oct 7, 2014

    @mgravell
    Member

    "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

  11. cincuranet commented on Oct 7, 2014

    @cincuranet

    Like that it will not work with Dapper correctly.

  12. mgravell commented on Oct 7, 2014

    @mgravell
    Member

    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

  13. mgravell commented on Oct 14, 2014

    @mgravell
    Member

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions