Skip to content

Mz fixes#56

Open
misak113 wants to merge 2 commits intoBurntSushi:masterfrom
misak113:mz-fixes
Open

Mz fixes#56
misak113 wants to merge 2 commits intoBurntSushi:masterfrom
misak113:mz-fixes

Conversation

@misak113
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Owner

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

@jezek
Copy link
Copy Markdown

jezek commented Jan 21, 2021

@BurntSushi
Copy link
Copy Markdown
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