psewar / Raidfelden.Discord.Bot

A Discord Bot built to add Crowd Sourced raids and pokemon to Monocle maps
MIT License
1 stars 1 forks source link

SaveChanges is not used safely w.r.t. database concurrency exceptions #5

Open bjmnbraun opened 6 years ago

bjmnbraun commented 6 years ago

--- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.d79.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.d77.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.EntityFrameworkCore.DbContext.d52.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Raidfelden.Data.GenericRepository`2.d23.MoveNext() in /root/Raidfelden.Discord.Bot/Raidfelden/Data/GenericRepository.cs:line 144 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Raidfelden.Services.RaidService.d22.MoveNext() in /root/Raidfelden.Discord.Bot/Raidfelden.Services/RaidService.cs:line 136 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Raidfelden.Services.RaidService.d18.MoveNext() in /root/Raidfelden.Discord.Bot/Raidfelden.Services/RaidService.cs:line 83 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Raidfelden.Services.RaidService.d17.MoveNext() in /root/Raidfelden.Discord.Bot/Raidfelden.Services/RaidService.cs:line 75 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Raidfelden.Services.RaidService.d16.MoveNext() in /root/Raidfelden.Discord.Bot/Raidfelden.Services/RaidService.cs:line 51 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Raidfelden.Discord.Bot.Modules.BaseModule`2.d29.MoveNext() in /root/Raidfelden.Discord.Bot/Raidfelden.Discord.Bot/Modules/BaseModule.cs:line 88 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Raidfelden.Discord.Bot.Modules.RaidModule.d8.MoveNext() in /root/Raidfelden.Discord.Bot/Raidfelden.Discord.Bot/Modules/RaidModule.cs:line 120

Fixing this requires wrapping the uses of SaveChanges in a try catch block that resets the "OriginalValues" of all exceptional entities with the database values. This kind of concurrency exception occurs when people add an OCR raid at the same time someon adds one on the map -- we only saw this once we started really ramping up usage.

psewar commented 6 years ago

Thank you for reporting this issue. Concurrency issues are always a hassle to fix and in this case I'm not sure if it's really worth to do anything about it. In the end even when successfully fixed it would not change anything in the database, as the same values which would be inserted by the bot have already been manually added by the user on the map. So of course running into exceptions isn't cool, but fixing it changes nothing in the end.

I'll leave this issue open for the moment as a reminder, to perhaps "fix" it by just ignore the exception

bjmnbraun commented 6 years ago

I have a patch that fixes, will upload.

The issue is not the exception, which is currently caught and doesn't take down the bot. It's that the nature of this exception is such that once you hit one of these every call to SaveAsync will throw the same exception. The bot is essentially useless until you completely restart it with ctrl c.

This is because of the way object proxies work in entity framework. Handling these exceptions by either choosing database or local values takes just a line or two, but you can't ignore the exception. It's something of an annoyance with using this kind of framework.

bjmnbraun commented 6 years ago

patches.tar.gz

Should handle concurrency errors in 3 places.