Closed deivid-rodriguez closed 5 years ago
kares
commented
5 years ago fine with this, since this isn't a hot path (in a production run) we'd like to avoid exception raising. any objections @enebo ?
@deivid-rodriguez you might want to target 50-stable if you'd like this in next 50.x 51.x 52.x releases
enebo
commented
5 years ago @kares I don't have any objection in the sense this probably is not super common and will only pay a penalty in those particular circumstances. Otherwise we will need to lock access and I feel like that will be a higher per use penalty.
I guess if this is really common then perhaps we do something else? Is it though?
kares
commented
5 years ago I guess if this is really common then perhaps we do something else? Is it though?
pbly not - the reported use-case is pretty much artificial ... its a test-case run
enebo
commented
5 years ago @kares yeah then we won't sweat it until we see -Xlog.exceptions and notice it is happening too much :)
deivid-rodriguez
commented
5 years ago Yeah, if I understood the error correctly, this is happening when creating the db folder, so this code path should probably just be hit once for the whole life time of a production application.
deivid-rodriguez
commented
5 years ago @deivid-rodriguez you might want to target 50-stable if you'd like this in next 50.x 51.x 52.x releases
Sure, yeah. Activeadmin currently supports all of those, so ideally we would want the change backported to all stable branches. I just wanted to start with master to agree on what the best fix is for this, then I can open backports :+1:.
deivid-rodriguez
commented
5 years ago Yeah, if I understood the error correctly, this is happening when creating the db folder, so this code path should probably just be hit once for the whole life time of a production application.
Actually, this is the connection method so I guess this happens once every time we connect to the DB? What I don't understand is that in our specs this folder should've already been created by the time the exception happened. So, I don't understand how this happened.
deivid-rodriguez
commented
5 years ago I got this error again and dug in a bit more but I'm still very confused.
The backtrace is now slightly different since now it shows an absolute path in the error message:
Errno::EEXIST:
File exists - /root/project/db
(...)From reading the surrounding code, it sounds like Rails.root is not yet defined at that point, that's why it tries to read a configuration from the current directory instead of from the actual Rails root (the test application at /root/project/tmp/test_apps/rails_50).
But since RSpec first loads all tests files before trying to run them and I'm requiring rails_helper on top of all of them, and that initializes the test application, I don't understand how that would happen.
deivid-rodriguez
commented
5 years ago I never experienced this again, and I don't like this "solution" anyways, so closing!
While testing activeadmin, our parallel test suite run into the following error:
As I understand it, this is a race condition where two threads would check
File.directory?(dirname)and both get a negative answer because the directory does not yet exist, and then both try to create the directory, causing the last thread to crash.This PR attempts to fix the error by rescuing the potential exception caused by the race condition.