Fix window creation thread handling

[andydotxyz]

This removes the races from handling multiple windows

Progresses #4654

Checklist:

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

[Jacalz]

Nice. I added some notes inline.

[Jacalz]

This seems like a hack to avoid waiting. I think it would be cleaner and likely quite a bit faster to just have a version of runOnMain that doesn't wait for the function to complete.

[andydotxyz]

Hmm, not to avoid waiting but to avoid blocking. Window creation happens when the main thread kicks off so waiting here would deadlock.
Does the request still stand? we can have a no waiting runOnMain, but I think it's specific to this line...

[Jacalz]

Not waiting on it to finish and not blocking is the same thing though, isn't it? Right now you are spinning up a new goroutine just to have runOnMain wait for the queued function to finish.

[Jacalz]

As I understand it, the runOnMain function queues the function to run on main and then waits for main to report back that it has completed. If we have a version of runOnMain that doesn't wait for main to report back, we would achieve the same non-blocking call as we do here but without the performance penalty (allocating a new stack and scheduling overhead) of launching a new goroutine.

[andydotxyz]

It has two code paths... when the app is running it can take the path of queuing the event and not waiting.
However before then the function is called directly. So to avoid that deadlock I have to move the goroutine into that instead. In that case we have more complex code to handle the possibility of a nil done but have still not eliminated the goroutine.

Having looked into it to make that summary I'm not sure it's worth it. The overhead is only during a window show which isn't exactly called often.

[Jacalz]

My mental model of the code tells me that this should be rather simply but I'll have a look at the code and see if I can put something simple together. I think we might just not be on the same page about what I mean. My mental model may of course also be entirely wrong... 😅

[Jacalz]

Hmm. You are right about the blocking vs waiting, I think.

However, I am a bit confused by this code. It seems like the initial show of the window happens before we are running so runOnMain() is running the function directly instead of queuing it onto main. This does however seem entirely counter intuitive when you are launching the function within a goroutine because now the function is actually called on the goroutine and not actually on main (unlike what the function name tells you). This does not seem to be intended? It should likely induce races.

Change this line to go w.doShow() and you will see cmd/hello still works exactly like it is supposed to. fyne_demo does not work but I suppose it is showing a second window after the main thread is started?

[coveralls]

coverage: 59.654% (-0.004%) from 59.658%
when pulling ea1ee73 on andydotxyz:fix/mergethreads
into d2fde27 on fyne-io:develop.

[andydotxyz]

Those window tests are surprisingly fiddly!

[Jacalz]

I seem to be getting this crash occasionally on Linux. It appears that some events are not running on the correct thread sometimes?

X Error of failed request:  BadAccess (attempt to access private resource denied)
  Major opcode of failed request:  150 (GLX)
  Minor opcode of failed request:  5 (X_GLXMakeCurrent)
  Serial number of failed request:  339
  Current serial number in output stream:  339

[andydotxyz]

Can you provide any info to help narrow down what/when/why?

[Jacalz]

There was not much to it really. Just starting the app on Linux (Intel iGPU on an Ultra 7 155h) crashed with that error randomly before managing to show a window. Happens maybe one in ten starts or something like that. I saw it in fyne_demo

I wonder if it is related to what I commented about runOnMain executing on the current thread/goroutine before the runloop has started? The error seems to indicate that some call isn't getting the right context.

[Jacalz]

Works great now, thanks. I still kind of think the documentation for runOnMain should state that it runs the function on the current thread if the run loop hasn't started but I won't hold this up on that change.

[andydotxyz]

Works great now, thanks. I still kind of think the documentation for runOnMain should state that it runs the function on the current thread if the run loop hasn't started but I won't hold this up on that change.

I was holding back on this because we need a new API here anyway - its a later checklist on the work to be done here :)