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

Mz fixes #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Mz fixes #56

wants to merge 2 commits into from

Conversation

misak113
Copy link

@misak113 misak113 commented Jan 4, 2021

I prepared some small changes which prevent some edge cases of XGB usage.

I can reproduce the panic errors on Xvfb connections when Xvfb is killed externally.
These changes will keep Go app alive just with a closed X server connection.
There are still some questions on running go routines in BG (see new TODO comments).

I'm free to discuss changes and update PR.

* wait to have successfully closed connetion to X server before event loops is stopped
* it's returned instead of continued because there are no reason to continue event loop when connection to X server is closed anyways
@jezek
Copy link

jezek commented Jan 5, 2021

Hello @misak113

I see you are trying to fix some memory leaks and panics on duplicated close.

I've made some work on these problems which resulted in PR #44 . Unfortunately the changes are not simple and the maintainer didn't trust my changes, so it's not merged (yet). That's why I wrote some test (#45 ) to be able to test panics and memory leaks on various occasions. If you want to continue your work on your solution, you can use the tests. I've tried to test your changes and found out, your solution always hangs on double close (and sometimes on simple close).

...
=== RUN   TestConnOnNonBlockingDummyXServer/close
    TestConnOnNonBlockingDummyXServer/close: testingTools.go:106: after server close, before testcase exit: github.com/jezek/xgb.(*Conn).generateSeqIds(0xc00025e000) is leaking
    TestConnOnNonBlockingDummyXServer/close: testingTools.go:106: after server close, before testcase exit: github.com/jezek/xgb.(*Conn).generateXIds(0xc00025e000) is leaking
=== RUN   TestConnOnNonBlockingDummyXServer/double_close
XGB: xgb.go:147: Connection was closed already: close of closed channel
^Csignal: interrupt

@BurntSushi
Copy link
Owner

Thank you for the PR, but as @jezek said, I just don't have the time to properly maintain this package. If someone else wants to start a fork, I'd be happy to deprecate this and link to it in the README.

@jezek
Copy link

jezek commented Jan 21, 2021

@BurntSushi

I just don't have the time to properly maintain this package. If someone else wants to start a fork, I'd be happy to deprecate this and link to it in the README.

I gave it a though and I wouldn't mind to take this weight from your shoulders.

@BurntSushi
Copy link
Owner

@jezek Thanks! Do you have a link to your fork?

@jezek
Copy link

jezek commented Jan 21, 2021

@BurntSushi
Copy link
Owner

@jezek Thanks! deaf085

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.

3 participants