Don't initialize plugins and user settings in tests #3220
Conversation
|
My bad that I suggested removing |
|
I had already a bad feeling with the removal of
But these are all internal/default plugins and I suppose it fails to load/initialize them. All logs end somehow with: The amount of
The out of bounds access was equal with the same index. Unfortunately it doesn't exactly tell were...if it was the line count which changed in the meantime or what ever. The MacOS build differs in the build arguments and one additional library loaded... ...which is needed by the |
|
Yeah, you are right, it would be good to find the root cause. And yeah, internal plugins shouldn't cause issues. So it's probably a good idea to test with plugins loaded, at least in some tests...
FWIW, at least once I had a different runtime error (not an out-of-range access but an unlock of a not-locked mutex), but also somewhere in the highlighter accessing the line array: |
|
Ok, the good news is that we can reproduce this locally. Just enter And as I've found, these 2 problems (
However, it seems to me it's not a good idea to load plugins in In order to initialize Lua correctly, it seems, we need to be inside So my current conclusion is that it's a good idea to do
|
Ok, that is a good argument against using it there.
ACK
I'll have a look at this too. |
|
Could it be possible that the current buffer under test is closed while to go routine at the end of Maybe it's much more trivial...
|
Yeah, and in particular, the problems that we are seeing are probably caused by deletion of lines: the number of lines changes (decreases), so |
|
BTW now it looks like #3208 per se is ok, it just (in combination with |
That's exactly what I meant. 😉 Sadly I've no backtrace of these two failing tests, since I forwarded the test output to Performed: Result: So now slightly different, it's the It's a wonder, that it wasn't found so far. |
Yeah, that explains it. The number of lines does not decrease, so So perhaps if you lock your per-buffer mutex during line insertions as well, perhaps you will not see a problem even after 100k runs... However I think that still would not be enough. I think we should synchronize any operations that modify the buffer, i.e. even those that only modify the contents of a single line, without changing the number of lines or their order. If one goroutine reads a byte array which is being modified by another goroutine without synchronization, I think there is no guarantee that it will work correctly. |
|
BTW a couple of times I saw this test not crashing but just hanging. It was just stuck somewhere, without printing any errors. |
Yeah... Unless we wanna change the entire approach to highlighting (i.e. unless we wanna speed up highlighting not by running it in a background goroutine but in some other way), it seems we do need such a per-buffer mutex, and we should hold this mutex:
|
|
But we should consider performance implications of such coarse-grained locking. Luckily, |
|
And for the future, all this suggests that doing EDIT: or alternatively, instead of creating a temporary empty config dir, just add a parameter to |
|
I prepared something to start with the lock on per buffer level: From my point of view the per line locks in the PS: |
|
So, 10k now passed. In the moment you wrote about the per-buffer lock did you mean the |
Yes, that's exactly what I was going to say in a reply to your previous comment. I said "per-buffer lock" as a figure of speech (to contrast it with a per-line lock), I actually meant a per-linearray lock. There are quite a few reasons why this lock should be in LineArray, and probably no reason why it should be in (Shared)Buffer.
Yep, I also think so. |
I wasn't asking for the per-line lock, just the layer of "per-buffer", but it's clear now.
Just try it with a lock inside |
I didn't say you were asking for it. :)
Hmm, good luck debugging. |
|
Maybe you already found it, but just in case, the deadlock is simply due to trying to lock the same mutex twice (in the same goroutine). Turns out that |
|
...So, a (hopefully correct) fix for this deadlock: Then there is another similar deadlock in deletion, so a (hopefully correct) fix for it: Then it works, but the buffer test still fails: so need to address that: and similarly in other functions used by the highlighter. This last point makes me think: so while this should (hopefully) guarantee correct and safe access to the line array, it does not guarantee that the highlighter itself will produce correct results (i.e. highlight correct pieces of text with correct colors), since the lines it operates on may be outdated. But HOPEFULLY such highlighting errors will be always immediately corrected by rehighlighting in |
No, because I was going to bed. 😉
I already had all of them in an intermediate version.
I've doubts too, since |
Yeah... For example, in Hmm... BTW what about |
The last one is tricky in case the test environment is out of your control.
This would prevent to mess around with the (current) three test environments. Sounds like the better choice, to force the intended set of features. Last but not least: I currently prepare a (hopefully) possible approach to get rid of our race condition. |
Adding InitPlugins() to tests has caused noisy error logs when running
the buffer_test.go test (although the test result is still PASS):
2024/03/23 15:14:30 Plugin does not exist: autoclose at autoclose : &{autoclose autoclose <nil> [runtime/plugins/autoclose/autoclose.lua] false true}
2024/03/23 15:14:30 Plugin does not exist: comment at comment : &{comment comment <nil> [runtime/plugins/comment/comment.lua] false true}
2024/03/23 15:14:30 Plugin does not exist: diff at diff : &{diff diff <nil> [runtime/plugins/diff/diff.lua] false true}
2024/03/23 15:14:30 Plugin does not exist: ftoptions at ftoptions : &{ftoptions ftoptions <nil> [runtime/plugins/ftoptions/ftoptions.lua] false true}
...
These errors are caused simply by the fact that plugins are initialized
but not loaded. Adding config.LoadAllPlugins() to buffer_test.go "fixes"
this problem.
However, at the moment it doesn't seem a good idea to load plugins in
buffer_test.go, since buffer_test.go doesn't properly initialize Lua. It
only does ulua.L = lua.NewState() but doesn't do the other stuff that
init() in cmd/micro/initlua.go does. As a result, plugins will not be
able to do anything correctly.
So in order to initialize Lua correctly we need to be inside cmd/micro/,
so we cannot do it in buffer_test.go or any other tests except
micro_test.go.
When initializing runtime files (syntax files etc) in tests, initialize built-in runtime files only, to ensure that the tests are not affected by whatever is in ~/.config/micro/ on the test machine. micro_test.go already ensures that, by using its own temporary directory as an (empty) config directory. So we only need to fix buffer_test.go and rtfiles_test.go. In those tests, don't repeat the same dance with a temporary directory, instead just ignore the config directory.
Adding InitRuntimeFiles() to buffer_test.go has changed the behavior of this test: now it tests not just buffer editing per se, but also how well buffer editing works together with syntax highlighting (since InitRuntimeFiles() loads syntax files, and many of the test buffers match the json header pattern in the json.yaml syntax file, so they are "highlighted" as json). This revealed long existing races between buffer editing and syntax highlighting. Until we fix those races, temporarily disable InitRuntimeFiles() in this test.
dmaluka
force-pushed
the
tests-rtfiles-fix
branch
from
4a6c1db to
69dc54b
Compare
Yep, just updated. In the last commit I commented out |
Adding
InitRuntimeFiles()andInitPlugins()tobuffer_test.goin PR #3062 (instead of just initializing runtime vars with empty values, as it was before) seems to cause sporadic failures of MacOS build tests on github, with crashes in various places but all beginning with lots of plugin failures:I suppose tests should not rely on plugins, and more importantly, should not be affected by the contents of ~/.config/micro/ on the host.