Skip to content
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

Fix several race conditions in internal/driver/glfw package #2048

Merged
merged 8 commits into from
Apr 8, 2021

Conversation

fpabl0
Copy link
Member

@fpabl0 fpabl0 commented Mar 1, 2021

Description:

Fix several race conditions in internal/driver/glfw package.
To test this fix, run:

go test -race -gcflags=all=-d=checkptr=0 fyne.io/fyne/v2/internal/driver/glfw

The flag -gcflags=all=-d=checkptr=0 is to temporarily ignore the crash related to gl (#1957).

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@fpabl0
Copy link
Member Author

fpabl0 commented Mar 3, 2021

After testing a lot the test called TestWindow_Focus (have written a script to run the test again and again until it fails haha) , I finally found where is the data race and in consequence why this test occasionally fails. The race occurs here:

w.charInput(w.viewport, 'd')
w.keyPressed(w.viewport, glfw.KeyTab, 0, glfw.Press, 0)

charInput(...) always enqueue the event to be called in a go routine later.
w.keyPressed with glfw.KeyTab event runs immediately in the main loop. Internally, it does w.canvas.FocusNext() that triggers the FocusLost method on widget.Entry and so refreshing the entry.Text and ignoring the charInput events because of the data race.

So, the question is should we also enqueue w.canvas.FocusNext() event from keyPressed to ensure all these operations are running sequentially in the same go routine?

@andydotxyz
Copy link
Member

So, the question is should we also enqueue w.canvas.FocusNext() event from keyPressed to ensure all these operations are running sequentially in the same go routine?

Yes I think that sounds correct

@fpabl0 fpabl0 marked this pull request as ready for review April 4, 2021 16:06
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like good improvements thanks

@fpabl0 fpabl0 merged commit 85855ac into fyne-io:develop Apr 8, 2021
@fpabl0 fpabl0 deleted the fix/race-glfw branch April 8, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants