Skip to content
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

Db.Async.execMany throws "A command is already in progress" with NPGSQL #30

Closed
MaxDeg opened this issue Aug 4, 2021 · 5 comments
Closed

Comments

@MaxDeg
Copy link

MaxDeg commented Aug 4, 2021

I'm using Donald with NpgSQL and find out that the Db.Async.execMany is throwing exception due to parallel execution of the commands. It's not occurring every time, probably depends on execution time of the queries.
The issue is probably not reproducible with SQLite (maybe it allows multiple command on the same connection in parallel).
I found the issue with NpgSQL but I'm 99% sure the behaviour is the same with SQLServer.

Issue is coming from this line: https://github.com/pimbrouwers/Donald/blob/master/src/Donald/Db.fs#L110
I cannot think of an easy way to fix it without adding task CE (ply or TaskBuilder). None CE dependent solution should probably rely on combining continuation recursively.
I would gladly help to apply a fix for it :)

Exception:
Npgsql.NpgsqlOperationInProgressException (0x80004005): A command is already in progress: INSERT INTO ingredient_months (ingredient_id, month_id) VALUES (@ingredient_id, @month_id)

Failing code:
do!
conn
|> Db.newCommand "INSERT INTO ingredient_months (ingredient_id, month_id)
VALUES (@ingredient_id, @month_id)"
|> Db.setTransaction tran
|> Db.Async.execMany months
|> TaskResult.mapError (FailedExecutionError >> raise)

Workaround:
for m in months do
do!
conn
|> Db.newCommand "INSERT INTO ingredient_months (ingredient_id, month_id)
VALUES (@ingredient_id, @month_id)"
|> Db.setParams m
|> Db.setTransaction tran
|> Db.Async.exec
|> TaskResult.mapError (FailedExecutionError >> raise)

btw: nice library, it's super pleasant to write infra code with it :)

@pimbrouwers
Copy link
Owner

Hey Max!

Thanks a lot for reaching out, and for looking into this. I tested this type of flow against SQL Server this morning and I got the exact same result, as you suspected. In reverting to 6.2.0 (pre-taskbuilder removal) everything was kosher.

I just added an internal copy of the widely accepted task builder and pushed it out to nuget (6.2.4). More importantly, I added a test for this specific case to ensure any changes we make going forward.

@MaxDeg
Copy link
Author

MaxDeg commented Aug 4, 2021

Wow super efficient :)
Thanks for the fix

@MaxDeg MaxDeg closed this as completed Aug 4, 2021
@pimbrouwers
Copy link
Owner

My pleasure!

I've had a sudden change of heart though about the implementation. I think it's likely a better idea to take an explicit dependency on say Ply for example.

I think this way we get all the patches the Nino pushes out, and it makes it more clear to consumers what this thing depends on.

Thoughts?

@pimbrouwers pimbrouwers reopened this Aug 4, 2021
@MaxDeg
Copy link
Author

MaxDeg commented Aug 4, 2021

Yep taking the Ply lib as dependency is better to keep up to date. Once F# 6 is out we may take advantage of the new task builder from core library :)
Only downside that I could see is that Ply is not the only task builder implementation outside. But on the other hand it's the most popular. Giraffe made the switch some times ago, I don't know what you are using for Falco

@pimbrouwers
Copy link
Owner

Thanks again for bringing this to my attention Max.

Closing since adding Ply to deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants