search

jbogard / ContosoUniversityCore

MIT License
591 stars 151 forks source link

DbContext transaction mechanism possibly swallowing exception? #1

Closed mderriey closed 7 years ago

mderriey commented 7 years ago

Hi Jimmy,

Looking at this code, I can't help but think it could swallow an exception. That catch statement calls RollbackTransaction, but doesn't throw.

Since the associated MVC filter rolls back if an exception is caught and rethrows, shouldn't CommitTransactionAsync just take care of committing the transaction and disposing of it if all works well, and let the MVC filter handle the exception scenario? Or at least rethrow the exception (that's what the non-Core version does.

Cheers

jbogard commented 7 years ago

Yep, that's a bug!

On Tue, Nov 8, 2016 at 10:41 PM, Mickaël Derriey notifications@github.com wrote:

Hi Jimmy,

Looking at this code https://github.com/jbogard/ContosoUniversityCore/blob/7d788093902f7c05e0dc94fc1e95122f28453edb/src/ContosoUniversityCore/Infrastructure/SchoolContext.cs#L47, I can't help but think it could swallow an exception. That catch statement calls RollbackTransaction, but doesn't throw.

Since the associated MVC filter https://github.com/jbogard/ContosoUniversityCore/blob/7d788093902f7c05e0dc94fc1e95122f28453edb/src/ContosoUniversityCore/Infrastructure/DbContextTransactionFilter.cs#L16 catches exceptions, rolls back if it's the case and throws, shouldn't CommitTransactionAsync just take care of committing the transaction and disposing of it if all works well, and let the MVC filter handle the exception scenario? Or at least rethrow the exception (that's what the non-Core version https://github.com/jbogard/ContosoUniversity/blob/a8d396f3df0f2db85ad8fc92125a1334cee015aa/src/ContosoUniversity/DAL/SchoolContext.cs#L61 does.

Cheers

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jbogard/ContosoUniversityCore/issues/1, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMjLo42lxfYEMuBz7LJLTee7PZr0Qks5q8OxzgaJpZM4Ks8PH .

mderriey commented 7 years ago

Sorry I didn't create a PR, I just wasn't sure this was an issue and I thought I could have missed something. Which, in hindsight, is stupid because the PR would have just not been merged. A lesson for next time, I guess.

Thanks!

jbogard commented 7 years ago

It took me an hour to debug this so lol

On Thu, Nov 10, 2016 at 9:57 PM Mickaël Derriey notifications@github.com wrote:

Sorry I didn't create a PR, I just wasn't sure this was an issue and I thought I could have missed something. Which, in hindsight, is stupid because the PR would have just not been merged. A lesson for next time, I guess.

Thanks!

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/jbogard/ContosoUniversityCore/issues/1#issuecomment-259805303, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMpuA0PIyUqnTJoR4rxH72Yvnb9eBks5q84UngaJpZM4Ks8PH .