Fix data races - Githubissues

nelsam / vidar

vidar is a highly experimental Go editor, written in Go, using gxui

The Unlicense

50 stars 7 forks source link

Fix data races #164

Closed Kvaz1r closed 5 years ago

Kvaz1r commented 5 years ago

I was trying to find reason or at least way to reproduce #162 but instead found a couple of data races.

One goroutine read from here and write from main goroutine.
Not safe access to h. layers from here.

It's a proposal for fix them.

Type

[x] Bug Fix
[ ] New Feature
[ ] Quality of Life Improvement

Tests

I have tested locally against:

[x] Windows
[ ] linux
[ ] OS X
[ ] BSD

I have included automated tests:

[ ] Unit tests
[ ] Integration tests
[ ] End to end tests

nelsam commented 5 years ago

@Kvaz1r I've made a series of changes to the code that's in need of goroutine safety to use unsafe.Pointer types with atomic.*Pointer functions. I'm not super happy with the code's layout, but I wanted to push up what I have (which appears to be working) for you to test. Can you try it out, see if it seems to be working alright for you? Also, let me know if this feels faster than the sync.Mutex code. If it isn't faster, then we're probably better off using sync.Mutex.

From what I tried, it feels snappy on my machines, but I want to make sure that it feels just as snappy for you.

Kvaz1r commented 5 years ago

yes, it works, I don't notice any problem with quick test.

nelsam commented 5 years ago

~After implementing some benchmarks, I found some outliers that caused a huge negative impact to performance, so I changed the implementation some. On my machine, the current implementation seems to average around 150ns in the most common scenarios and around 350ns for the slowest scenarios.~

~As long as I'm working in this code, I'm going to implement a few tests before merging; but feel free to test it out just to double-check that it's working for you.~

Ehrm... hold on. I'm seeing something like data races still.

nelsam commented 5 years ago

Okay, now I think it's ready. I must have been changing tabs while my edits were still processing, and since all didn't have any synchronization, something went wrong.

I still need to update the mouse clicks on the open tabs to also use the focus command, but that's another story.

nelsam commented 5 years ago

@Kvaz1r implementing tests actually uncovered a bug with the branching history, which is probably the problem I was actually running in to. It's fixed now.

Kvaz1r commented 5 years ago

cool, I also noticed earlier some problem with history but never can't found way to reproduce it.

nelsam commented 5 years ago

I made one final cleanup commit and am letting that hit CI before merging; but I'll be merging shortly after that :)