lxn / walk

A Windows GUI toolkit for the Go Programming Language
Other
6.78k stars 885 forks source link

General unstability #751

Open StephanVerbeeck opened 3 years ago

StephanVerbeeck commented 3 years ago

Hi all,

I do not have a current problem with LXN/WALK but I would like to offer some solid advice as to general stability and how to resolve these issues. Walk is using the underlying windows message API and that means that all GUI interactions have to be done by the same thread. You would think that this is solved in walk itself but there you are mistaken. So what follows here is my proposal to what needs to be changed in LXN/WALK to make this GUI environment suitable for commercial products.

1) There are no (better) alternatives. Walk is table and offers most of the functionality that we expect and need for writing any windows desktop program. 2) It is wrong to leave it to the human user to check that he is (as programmer) not violating the restriction that all message processing must be done on the same windows OS thread. 3) GOlang go-routines are insanely useful but they conflict with the above, and even if you don't . . . the libraries that you include wil use go-routines and cause havoc.

The symptoms of violating this prime rule vary but if you have unexplainable lockup of the GUI. program crashes. Programs that do not start. Programs that do start but do not display the mainwindow. Problems with tooltip messages . . .

All of these have always the same reason being that you violated the prime rule and did GUI interaction from multiple threads.

I propose to add the equivalent of the following 2 functions in lxn/walk and lxn/win.

    // GUI performs function on thread that runs the main-message-loop
    func GUI(windowInteraction func()) {
        if win.IsMessageThread() {
            windowInteraction() // perform action synchronous on this thread
        } else {
            mainWindow.Synchronize(windowInteraction) // perform action synchronous on other thread that handles windows GUI calls
        }
    }
    var (
        checkMessageThreadID uint32 // windows message loop MUST always be processed by the SAME thread
    )

    // CheckMessageThread windows message loop MUST always be processed by the SAME thread
    func CheckMessageThread() {
        if checkMessageThreadID == 0 {
            runtime.LockOSThread()
            checkMessageThreadID = GetCurrentThreadId()
        } else {
            Assert("win.CheckMessageThread", checkMessageThreadID == GetCurrentThreadId())
        }
    }

    // IsMessageThread return true when called by the thread that has runs the windows OS main-message-loop
    func IsMessageThread() bool {
        return checkMessageThreadID == GetCurrentThreadId()
    }
// call CheckMessageThread() from within win.SendMesssage()
    // call CheckMessageThread() from within walk.runSynchronized

    // call CheckMessageThread() from within defaultWndProc() in walk window.go

This will save your life and save your project deadline. Best regards and don't bet on it that you don't need this fixed O:-) Once the check is in there you will get an error for all your wrong deeds :-)

alkanna commented 3 years ago

I'm just starting to learn how to use this lib, and I find myself refactoring my code all the time because there is so little documentation. The code itself is quite well written and very easy to read though. I have not yet ventured into that kind of issue, but I would love to contribute at some point ! Why not make a pull request with the aforementioned changed @StephanVerbeeck ?

jscholes commented 3 years ago

You would think that this is solved in walk itself but there you are mistaken.

Not really. You mentioned the Synchronize method later in your comment, which seems to fulfill the definition of "solving" it.

It is wrong to leave it to the human user to check that he is (as programmer) not violating the restriction that all message processing must be done on the same windows OS thread.

I suppose the validity of this issue all revolves around this single question: why is it not the job of the programmer to impose clean separation between GUI and non-GUI code, and then ensure that the GUI portion is running on the main thread? Particularly if they're working in a professional, commercial context as you mentioned...

GOlang go-routines are insanely useful but they conflict with the above, and even if you don't . . . the libraries that you include wil use go-routines and cause havoc.

A library that I import probably shouldn't be communicating directly with my GUI controls. Again, separation is key.

I don't disagree that a win.IsMessageThread function might be useful to somebody. But you're still having to call your new GUI function in your program anyway, so you may as well just create a gatekeeper which always uses Synchronize and call it a day. There is no guarantee that just because you've created your GUI function, programmers on your team will use it, and if they need to learn that, why not just learn Synchronize?

StephanVerbeeck commented 3 years ago

You would think that this is solved in walk itself but there you are mistaken.

Not really. You mentioned the Synchronize method later in your comment, which seems to fulfill the definition of "solving" it.

Your statement contains a false assumption. Yes it is tested, but only if you did already got it right (by using the Synchronize function). All other 99% of message traffic is unshielded!

jscholes commented 3 years ago

Yes it is tested, but only if you did already got it right (by using the Synchronize function). All other 99% of message traffic is unshielded!

Sorry, I don't follow. If a programmer isn't aware of the Synchronize method and the need to use it, why would they write a version of your GUI function? They have to be aware of the need to use Synchronize to write and use GUI, but if they're aware of the need to use Synchronize why bother writing GUI?

StephanVerbeeck commented 3 years ago

Ah, ok good question. Well the Assert() will throw a runtime error and by looking at the stackdump the programmer can seen the place in the code where he had used walk functions while not being on the main program thread. Then the programmer must enclose that part of the code in a GUI call like this.

bad:

SetBookingsVisible(dir.HasAccountDatabase())

            go func() {
                mailContactTableModel.Load()
                mailContactTableModel.ResetRows() // fill tableview with mail contacts
            }()

good:

                GUI(func() {
                    SetBookingsVisible(dir.HasAccountDatabase())
                })
            go func() {
                mailContactTableModel.Load()
                GUI(func() {
                    mailContactTableModel.ResetRows() // fill tableview with mail contacts
                })
            }()

The silly thing is that doing it wrong will work 999 times before crashing. So you have the feeling being back in C/C++ without this security-belt. Such errors are very timing sensitive and go away and reappear by adding/removing a simple debug print statement driving you totally crazy. It is like shared write to a GOlang map without lock. Just once-in-a-while does it fail making you think you have made no error and then one day "boom".