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

XLib event loop canceling #16

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

data-niklas
Copy link
Contributor

This PR allows canceling the XLib event loop by sending a custom ClientMessage on an invisible Window.
See #15
For further XLib documentation refer to Tronche's documentation.

@changkun changkun self-requested a review May 10, 2022 07:11
Copy link
Member

@changkun changkun left a comment

Choose a reason for hiding this comment

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

Hi, thanks for this great contribution.

I just tested this change with an example:

func TestHotkey_Unregister(t *testing.T) {
	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	defer cancel()

	hk := hotkey.New([]hotkey.Modifier{hotkey.ModCtrl, hotkey.Mod2, hotkey.Mod4}, hotkey.KeyA)
	if err := hk.Register(); err != nil {
		t.Errorf("failed to register hotkey: %v", err)
		return
	}
	if err := hk.Unregister(); err != nil {
		t.Errorf("failed to unregister hotkey: %v", err)
		return
	}

	for {
		select {
		case <-ctx.Done():
			return
		case <-hk.Keydown():
			t.Fatalf("hotkey should not be registered but actually triggered.")
		}
	}
}

It actually crashes:

=== RUN   TestHotkey_Unregister
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x1 addr=0x968 pc=0x7f2ab8dac414]

runtime stack:
runtime.throw({0x534755?, 0x39?})
        /home/changkun/goes/go1.18/src/runtime/panic.go:992 +0x71
runtime.sigpanic()
        /home/changkun/goes/go1.18/src/runtime/signal_unix.go:802 +0x3a9

goroutine 18 [syscall]:
runtime.cgocall(0x4fa410, 0xc00006be28)
        /home/changkun/goes/go1.18/src/runtime/cgocall.go:157 +0x5c fp=0xc00006be00 sp=0xc00006bdc8 pc=0x4058fc
golang.design/x/hotkey._Cfunc_sendCancel(0x0, 0x0)
        _cgo_gotypes.go:110 +0x45 fp=0xc00006be28 sp=0xc00006be00 pc=0x4f86a5
golang.design/x/hotkey.(*Hotkey).unregister.func1(0x4657ee?)
        /home/changkun/dev/golang.design/hotkey/hotkey_linux.go:85 +0x56 fp=0xc00006be68 sp=0xc00006be28 pc=0x4f8cd6
golang.design/x/hotkey.(*Hotkey).unregister(0xc000146080)
        /home/changkun/dev/golang.design/hotkey/hotkey_linux.go:85 +0x8a fp=0xc00006beb8 sp=0xc00006be68 pc=0x4f8baa
golang.design/x/hotkey.(*Hotkey).Unregister(0xc000146080)
        /home/changkun/dev/golang.design/hotkey/hotkey.go:100 +0x25 fp=0xc00006bed0 sp=0xc00006beb8 pc=0x4f7f25
golang.design/x/hotkey_test.TestHotkey_Unregister(0xc0001024e0)
        /home/changkun/dev/golang.design/hotkey/hotkey_linux_test.go:52 +0x125 fp=0xc00006bf70 sp=0xc00006bed0 pc=0x4f9d05
testing.tRunner(0xc0001024e0, 0x537100)
        /home/changkun/goes/go1.18/src/testing/testing.go:1439 +0x102 fp=0xc00006bfc0 sp=0xc00006bf70 pc=0x4bb562
testing.(*T).Run.func1()
        /home/changkun/goes/go1.18/src/testing/testing.go:1486 +0x2a fp=0xc00006bfe0 sp=0xc00006bfc0 pc=0x4bc40a
runtime.goexit()
        /home/changkun/goes/go1.18/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc00006bfe8 sp=0xc00006bfe0 pc=0x4679c1
created by testing.(*T).Run
        /home/changkun/goes/go1.18/src/testing/testing.go:1486 +0x35f

goroutine 1 [select, locked to thread]:
golang.design/x/mainthread.Init(0xc000072500)
        /home/changkun/dev/golang.design/hotkey/vendor/golang.design/x/mainthread/mainthread.go:100 +0x16b
golang.design/x/hotkey/mainthread.Init(...)
        /home/changkun/dev/golang.design/hotkey/mainthread/os.go:19
golang.design/x/hotkey_test.TestMain(0xc0000aa1e0?)
        /home/changkun/dev/golang.design/hotkey/hotkey_test.go:19 +0x56
main.main()
        _testmain.go:53 +0x1d3

goroutine 6 [chan receive]:
testing.(*T).Run(0xc000102340, {0x52f9cb?, 0x4bb2c5?}, 0x537100)
        /home/changkun/goes/go1.18/src/testing/testing.go:1487 +0x37a
testing.runTests.func1(0xc00010e0f0?)
        /home/changkun/goes/go1.18/src/testing/testing.go:1839 +0x6e
testing.tRunner(0xc000102340, 0xc00006ad70)
        /home/changkun/goes/go1.18/src/testing/testing.go:1439 +0x102
testing.runTests(0xc0000aa1e0?, {0x5f5580, 0x2, 0x2}, {0x0?, 0x0?, 0x612dc0?})
        /home/changkun/goes/go1.18/src/testing/testing.go:1837 +0x457
testing.(*M).Run(0xc0000aa1e0)
        /home/changkun/goes/go1.18/src/testing/testing.go:1719 +0x5d9
golang.design/x/hotkey_test.TestMain.func1()
        /home/changkun/dev/golang.design/hotkey/hotkey_test.go:19 +0x1d
golang.design/x/mainthread.Init.func1()
        /home/changkun/dev/golang.design/hotkey/vendor/golang.design/x/mainthread/mainthread.go:96 +0x5b
created by golang.design/x/mainthread.Init
        /home/changkun/dev/golang.design/hotkey/vendor/golang.design/x/mainthread/mainthread.go:92 +0x10a

goroutine 19 [runnable]:
golang.design/x/hotkey.newEventChan.func1()
        /home/changkun/dev/golang.design/hotkey/hotkey.go:127
created by golang.design/x/hotkey.newEventChan
        /home/changkun/dev/golang.design/hotkey/hotkey.go:127 +0x92

goroutine 20 [runnable]:
golang.design/x/hotkey.newEventChan.func1()
        /home/changkun/dev/golang.design/hotkey/hotkey.go:127
created by golang.design/x/hotkey.newEventChan
        /home/changkun/dev/golang.design/hotkey/hotkey.go:127 +0x92

goroutine 21 [runnable]:
golang.design/x/hotkey.(*Hotkey).register.func1()
        /home/changkun/dev/golang.design/hotkey/hotkey_linux.go:74
runtime.goexit()
        /home/changkun/goes/go1.18/src/runtime/asm_amd64.s:1571 +0x1
created by golang.design/x/hotkey.(*Hotkey).register
        /home/changkun/dev/golang.design/hotkey/hotkey_linux.go:74 +0x165
exit status 2
FAIL    golang.design/x/hotkey  0.004s

hotkey_linux.c Outdated Show resolved Hide resolved
hotkey_linux.c Outdated Show resolved Hide resolved
hotkey_linux.c Outdated Show resolved Hide resolved
Co-authored-by: Changkun Ou <[email protected]>
@data-niklas
Copy link
Contributor Author

Thanks for testing the code! (It worked on my machine in my limited tests, haha). I will try to reproduce the error and stabilize the PR.

@data-niklas
Copy link
Contributor Author

I think that 'unregister' might be called before the ´Display´ and ´Window´ are initialized in the handle method.

@data-niklas
Copy link
Contributor Author

I added your test.

Comment on lines +86 to +88
if hk.display != nil {
C.sendCancel(hk.display, hk.window)
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't feel confident about this approach. What if the display is allocated after this check? What if the Unregister happens on a different goroutine to Register? This will lead to leaking resources, and race conditions of hk.display.

I think we will need some sort of synchronization here to guarantee that unregister can be called only after display and window are allocated.

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.

2 participants