-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
CommandTimeout on Async methods does not cause exception when exceeded #1247
Comments
The first thing to check is: does the same command, issued via raw ADO.NET, using the async API with timeout - work as expected, i.e. does it timeout? If it does, then it is probably a Dapper bug. If it doesn't, then there won't be anything Dapper can do! |
Yes I agree, and I will re-confirm it does not timeout (i.e., it's not a Dapper bug). I was just wondering if you observed this thru any of your tests, and have seen for any provider that it times out? |
It is a great question, and I don't have a definitive answer to hand. If
love to say "yes, absolutely", but I'm not at a PC to check if we have
explicit tests for this. The way I look at it there are three possibilities:
- it already works fine when providers implement it
- it should work fine when providers implement it, but there's a brain-dead
bug in Dapper that we need to fix
- it never works at all and will never work because ADO.NET doesn't support
it at a fundamental level
If it turns out to be the third option, I'd be open to making Dapper do the
hard work, meaning: adding the required timeout logic into Dapper. If it is
the second option, we should obviously fix the bug. The trickiest is the
first option, as then we have something that currently may or may not work
depending on the provider.
…On Fri, 26 Apr 2019, 23:10 Janet Roberts, ***@***.***> wrote:
Yes I agree, and I will re-confirm it does not timeout (i.e., it's not a
Dapper bug). I was just wondering if you observed this thru any of your
tests, and have seen for any provider that it times out?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1247 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAEHMFMIQMBERKRYFSY3SLPSN4URANCNFSM4HI2FP2A>
.
|
Again, thank you for the prompt and thoughtful reply! I will make it a priority to get you some feedback. If indeed it's the 3rd option and you can "fix" it in Dapper, what do you think would be the timeframe for that? |
the tricky thing isn't the code specifically - it is trying to think about every possible path through the code and what every provider does/doesn't support - I don't think it is huge, though - I'm very familiar with async timeouts etc |
Looks like it might be option #1 !! I have 4 tests SqlServer/NpgSql using Ado/Dapper. The 2 SqlServer tests DO timeout, the 2 Npgsql tests do NOT timeout. So barring some brain-dead code in my tests, it appears to be an Npgsql issue. I have not tested all the methods, just ExecuteReaderAsync. So I have posted a question to that team: npgsql/npgsql#2457. UPDATE: Yes we have confirmed that sql server honors the timeout and postgres ignores the timeout. Definitely something useful to know :-) Feel free to close this issue, and thank you for your time! |
I would like to know if you have successfully timed out any Async call for any data provider?
The timeout (as indicated by MSDN) is not working for me using:
Dapper 1.50.5
Npgsql 3.2.7
.Net Core 2.1.2
The following sample postgres query does not timeout (e.g., with Delay = 5, Timeout = 2):
SELECT i.Id, i.Name
FROM public.Item i, pg_sleep(@delay) -- to simulate long-running query
WHERE Id = @id;
Using calls similar to this simplified snippet:
cd = new CommandDefinition(
commandText,
parameters,
commandType: CommandType.Text,
commandTimeout: Timeout,
flags: CommandFlags.None,
cancellationToken: cancellationToken
);
QuerySingleOrDefaultAsync<Item>(cd);
I use the CommandDefinition signature so that i can pass the cancellation token.
If indeed the underlying timeouts are not supported (e.g., if not a Dapper problem), where would one have found documentation for this?
Thank you in advance,
Janet
The text was updated successfully, but these errors were encountered: