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

Dispose of connection from dbcontext. #2

Merged
merged 2 commits into from
Dec 15, 2022

Conversation

johngrant
Copy link
Contributor

No description provided.

@johngrant
Copy link
Contributor Author

The connection pool became exhausted in my app until I adequately disposed of the connection in the DB context.

@pimbrouwers
Copy link
Owner

Hi John!

Thanks for submitting this.

I am confused though. The Reset method of the DbContext is (or should be) doing that for you when you commit or rollback. Are you absolutely sure that you are calling one of those?

@johngrant
Copy link
Contributor Author

johngrant commented Nov 28, 2022

The Reset would indeed do this too, but what about requests that don't perform updates? Maybe the caller is only reading a list of items from the data source.

During my tests, I found after about 100 reads, DbContext starts returning connection pool limit reached errors.

To me, it doesn't make sense to call commit() when doing a read. Also, a popular pattern is to keep all CRUD operations related to a single entity in the same repo, which would get the injected context. The injected context would be used for reads and updates.

So, how do you account for closing the connection when the caller only reads data?

@pimbrouwers
Copy link
Owner

Thanks John.

There is no discerning about what kind of operation is occurring during the batch/unit. The presumption is that Commit() or Rollback() is invoked after every interaction. There is no "auto-committing".

See this StackOverflow answer for the original answer to this: https://stackoverflow.com/a/50261865/2421277

@pimbrouwers pimbrouwers reopened this Nov 29, 2022
@johngrant
Copy link
Contributor Author

johngrant commented Nov 29, 2022

This isn't auto-committing. This change is disposing of an exhaustable resource properly.

You don't implement IDisposable in UnitOfWork nor in DbContext which is the source of the bug. There is already one issue brought up by another developer who is running into this same problem.

@pimbrouwers
Copy link
Owner

The library does dispose of the resource properly, provided a commit or rollback is invoked after every interaction. This is the intended usage. The non-implementation of IDisposable is purposeful. It is not a bug.

I don't know how else to explain this.

@johngrant
Copy link
Contributor Author

johngrant commented Nov 29, 2022

Can you give a good reason not to implement IDisposable inside DbContext or UnitOfWork?

@johngrant
Copy link
Contributor Author

What about scenarios where a request is serviced and a response formulated from static data in memory and the database is not used? The connection factory and connection open still gets run, but again it is never disposed of properly.

@MARKUSER
Copy link

MARKUSER commented Dec 1, 2022

Thanks @johngrant for looking into the issue and the fix. I couldn't use this package due to the issues mentioned in my ticket but when i get some time i will rework on my solution with latest fix and update here. Thanks for your effort and time.

@pimbrouwers
Copy link
Owner

pimbrouwers commented Dec 7, 2022

Hi @johngrant,

I got an email notification today from this thread, it looks like you might have deleted the message, but you seemed quite angry. So I wanted to touch base.

Firstly, I hope there aren't any hard feelings. I'm not trying to put up road blocks. Admittedly, this code is quite old. I wrote it originally in 2018. So I'm sure if I went back through it, I would probably find a bunch of other things that I wouldn't like anymore either. Heck, this isn't even my primary language anymore haha!

That said, I do want to reiterate that the DbContext is intended to be a black box. I don't want the runtime to dispose of anything implicitly. The problem is, what do we do in the case there is an open unit? Do we commit? Rollback? The point being that we have to "do" something.

If we are going to add this, then I would advocate for it calling Commit() which would save the unit, rollback if there was failure, and dispose of everything.

In the case where the consumer doesn't use the connection, nothing should happen. The DbContext uses lazy auto properties. It's a pattern I wouldn't say I love anymore. But it is effective in that nothing happens if you don't observe it. So the intention here was that, if you speaketh my name, you cleaneth me up.

@johngrant
Copy link
Contributor Author

johngrant commented Dec 13, 2022

Well, I didn't want to throw shade so I decided to delete the message.

There is a bug here.

In my tests, the connection factory code that opens the connection is run per every request.

Typically in an API every API call doesn't mutate (update, delete, create) the data, so a transaction doesn't make sense.

So even for a simple read (select * from Items), I will have to call Commit(), or else the connection pool will quickly become exhausted, and the application will throw 500 errors.

It took me running a Postman test in a loop to find this because

  1. There is no logging or error handling in the connection factory in case the connection open fails.
  2. It takes about 100 connection opens. That is a lot for a single developer but not in a QA or production environment.

I found this bug from my mobile developers complaining about non-descript server 500 errors.

It makes zero sense to call a commit() method for a single read. Commits are for updates or inserts or several wrapped in a transaction.

@pimbrouwers
Copy link
Owner

I am glad you elected to go that route. We are all here for fun (and profit).

I mean this in the nicest way possible, this is not a bug. You yourself even go on to say "it makes zero sense", which frames this correctly, your opinion. My intent was that Commit() is called after all interactions. I have said this multiple times now. Bugs are anomalies in the code, this is the intended usage of the library. I will admit, this should be better documented, that is on me. I will fix that as soon as we get this change in.

That aside, I don't understand why you keep pressing? I agreed to make your change. I just advocated for the method to call Commit() instead. The change in this PR is actually a bug, in that it pays no mind to the potentially open unit of work, as I stated above. The worst part about it is that certain vendors may not actually throw an exception (some will), but rather rollback the transaction. If you're reading only this ultimately means nothing as the consumer. But the "ticker tape" of the database would have been written to multiple times unnecessarily, which you don't want.

The point of all interactions being done in an explicit transaction, is that there is a worthwhile performance gain since it saves the engine work. So we definitely want to keep this.

Update the file to call Commit(), I'll merge, update the docs and deploy a new patch version of the NuGet package. And let's move on with our lives.

@johngrant
Copy link
Contributor Author

Thanks. I didn't see where you agreed to a change or misread it. I updated the PR to call commit during disposal instead.

I appreciate the library. It came in handy, except for the hiccup with the connection pool becoming exhausted. I think we discussed it enough. Feel free to accept or reject the PR. Thanks again!

@pimbrouwers pimbrouwers merged commit fb3485b into pimbrouwers:master Dec 15, 2022
@pimbrouwers
Copy link
Owner

My pleasure! Thanks for the discussion. Will package and deploy this now.

@pimbrouwers
Copy link
Owner

pimbrouwers commented Dec 15, 2022

@MARKUSER @johngrant

This has been deployed as part of v2.1.1.

@pimbrouwers pimbrouwers added the enhancement New feature or request label Dec 15, 2022
@pimbrouwers pimbrouwers self-assigned this Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants