Export to GitHub

dapper-dot-net - issue #106

Error while iterating rows results in TimeoutException


Posted on Jul 6, 2012 by Happy Monkey

This applies only to the case where you do Query(buffered: false). Non-buffered queries basically return a IEnumerable over the DbDataReader.

The problem goes like this: If during your processing of the results, e.g. inside a foreach(var row in connection.Query("...", buffered: false)) an exception is raised, the thread seems to freeze for sometimes and then raises a Timeout exception on the connection. Raising an exception inside the loop could be an expected use-case, unexpected error, or simply cancelToken.ThrowIfCancellationRequired (which could be a common use-case if you have lots of row).

The reason behind is this: This happens with SQL Server if you call Close() (which is called by Dispose()) on a datareader before it has read all its data. Apparently it's linked to the networking protocol. For your reference, here's a discussion of the problem: http://stackoverflow.com/questions/133374/net-sqldatareader-close-or-dispose-results-in-timeout-expired-exception

The solution: The official, recommended solution is to call Cancel() on the DbCommand before disposing the DbDataReader.

Suggested fix: inside QueryInternal() instead of simply wrapping the reader with: using(var reader = cmd.ExecuteReader()) use this: try { // ... existing code ... reader.Close(); } finally { if (!reader.IsClosed()) cmd.Cancel(); reader.Dispose(); }

Comment #1

Posted on Jul 6, 2012 by Happy Monkey

I would like to add that an exception is not strictly required. Quiting a foreach early should exhibit the same behavior:

foreach (var row in conn.Query("...", buffered: false)) { if (row.Value == "That's what I want!") break; }

Comment #2

Posted on Sep 19, 2012 by Happy Monkey

Given that I did provide both the problem and a fix, is there any chance to see that code merged into Dapper proper?

I'm currently using my own modified version of Dapper, which is kind of annoying to keep in sync with new releases.

Comment #3

Posted on Sep 19, 2012 by Massive Bear

(No comment was entered for this change.)

Comment #4

Posted on Sep 19, 2012 by Massive Bear

Aplogies; I will look at this for the next deploy. Or you have my permission to hunt me down and yell at me!

Comment #5

Posted on Oct 9, 2012 by Helpful Cat

This seems to have caused a regression error for the Dapper.Contrib tests, and it is also causing issues for Postgres.

The SqlCe implmentation of IDbCommand doesn't appear to implement Cancel() at all, and throws an exception when called.

As for Postgres, after a short time of continually running queries, a 'user requested cancel' happens for the currently running query, from what appears to be a previous query's cancel. I still haven't tracked down if it is Dapper or Npgsql causing it.

Comment #6

Posted on Oct 9, 2012 by Massive Bear

I'll add some try/catch around the Cancel, which should fix SqlCe; can you be more specific about the Postgres? I'm unclear what is happening there.

Comment #7

Posted on Oct 9, 2012 by Helpful Cat

I'm still trying to track down what is going on, but I altered the test suite for the contrib section to run for both CE and Postgresql. The CE section failed from the non-implemented cancel. Postgresql randomly failed while executing some statements saying "ERROR: canceling statement due to user request". If I comment out the section that does the cmd canceling, it runs fine.

If I can't track down the bug, I will sumbit the work that I've done and someone else can have a go.

Comment #8

Posted on Oct 10, 2012 by Helpful Cat

Have submitted a pull request with a small patch that should at least fix up Postgres.

I'm still unsure of what is going on in its entirety, but I think there is a race condition between the connection being torn down and disposed of, and the previous connection's cancel command.

If you're interested in having a look, I've attached the part of the crash log that might explain some of what was going on. It has a lot of the logging from Npgsql turned on and some custom stuff. :D

Attachments

Comment #9

Posted on Oct 10, 2012 by Massive Bear

See pull request notes; applied manually, but with some minor tweaks - you may want to check that this still fixes Postgres.

Comment #10

Posted on Oct 10, 2012 by Massive Bear

Comment deleted

Comment #11

Posted on Oct 10, 2012 by Helpful Cat

Seems to work fine for my test cases, and no spurious command cancels. Looks good.

Status: Accepted

Labels:
Type-Defect Priority-Medium