-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Reload schema definition on SIGHUP #570
Conversation
<> show minimumPgVersion) | ||
getDbStructure (cs $ configSchema conf) | ||
|
||
dbStructure <- newIORef $ either (error.show) id result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call it refDbStructure like in App.hs
I know there is no big chance for "race conditions" here but what are the advantages of IORef over MVar? My understanding is that MVar is a IORef with "locks attached" |
That's a good question about MVar vs IORef. The code on this branch initially used MVar, but it seemed like it wasn't adding any safety for the situation over IORef so I thought I'd use the simplest thing that works. The only perceived advantage is simplicity. |
The question is for my own education, not that there is anything wrong with the code. |
Maybe another way to put it is "is there any time a good reason to use ioref vs mvar" and it seems there might be, some discussion about performance here (didn't look too closely) https://www.reddit.com/r/haskell/comments/39ef3y/ioref_vs_mvar_vs_tvar_vs_tmvar/ |
IORef does less internally and is a little faster, not that it matters much. I found a good comparison between the two abstractions: 1, 2. I think we should be safe from concurrency problems because I'm assuming db changes and sighup happen seldom whereas web requests happen often.
Also MVar models the distinction of being full or empty, whereas our IORef obtains a value upon creation and it doesn't make sense for it to be "emptied." |
I loved the pull request and the thread here, very informative :D |
Fixes #475
I manually verified this works, but it's hard to think how to add an automated test.