Don't use logger.Fatal

[Merovius]

imho there is absolutely no excuse to use logger.Fatal in a library. logger.Fatal should only be used for debug-purposes (like an assert or something) or in an application where all behaviour is well-known. logger.Fatal calls os.Exit which in turn prevents other cleanup-operations (like removing files or similar). Therefore every well-written software should always return from main. Also, not every application that uses X should be considered „unrecoverable“ if the X-connection dies. (Consider a C-library that calls _exit(2) - not even exit(3) - which would almost certainly have undesirable side-effects).

The easiest „correct“ way to handle this would probably be to panic in the deeper nested calls and recover in the user-facing API-calls and gracefully returning an appropriate error.

[BurntSushi]

First let me say that I agree.

The easiest „correct“ way to handle this would probably be to panic in the deeper nested calls and recover in the user-facing API-calls and gracefully returning an appropriate error.

It's not as simple as that. The two calls to Fatal in XGB are write or read errors to the X socket, which occur in goroutines that don't have any direct interaction with the user. Unless you have another idea, I could settle on changing the calls to Fatal to panic with the error returned by Write or ReadFull. This would at least allow the caller to recover in main, I think.

[Merovius]

panicking in a different goroutine will crash the program. The clean way would be to return an error to the user in Conn.WaitForEvent or Conn.PollForEvent or similar. I'm not familiar with the codebase, so I'm not sure if this is possible at all without major refactoring. Might take a look later.

[BurntSushi]

panicking in a different goroutine will crash the program.

Unlike Fatal, it is recoverable by the client.

The clean way would be to return an error to the user in Conn.WaitForEvent or Conn.PollForEvent or similar. I'm not familiar with the codebase, so I'm not sure if this is possible at all without major refactoring. Might take a look later.

I've thought about this more. Not only will it be quite difficult to do so in the existing code base, but I think it's wildly inappropriate. A read/write error with the X connection is truly exceptional, whereas standard X errors are quite commonplace. In the vast majority of cases, an error with the X connection is substantial. Similarly, in the vast majority of cases, a standard error returned by X is mundane and happens all of the time.

Practically speaking, this would require undue error checking by the client. It is often the case that an X error can simply be ignored or worked around; but a connection error should never be ignored.

In the minority of cases where you need to recover from a connection error, the client can use recover.

[Merovius]

If the panic happens in a different goroutine, a recover is not possible, as far as I understood. In the Article of the goblog describing defer, panic and recover it says

The process continues up the stack until all functions in the current goroutine have returned, at which point the program crashes.

Regarding recovering in the client, the article also states, that this is very un-go-ish:

The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.

Regarding the complexity of the change: A very cursory reading of the source suggests, that it would suffice, to change some return values from the type Error to the builtin error-type and writing the Read-error to the Conn.eventChan of the current connection. Haven't tried this yet, but is there an obvious fault with that, which I am missing?

Regarding the X-Errors vs. Connection-errors I agree with you, that X-Errors are a very common thing, but I disagree with you, that they should be ignored because of that. They are a lot like warnings, just because you can ignore them without immediately crashing your program, doesn't mean you should ignore them - they still tell you that there is some flaw in your program, some expectiaton not met or some calculation not right and you should fix them. To make this more than ”just my opinion“: This is very much along the spirit of go, where there are no warnings and clean coding is built into the language. Besides, with above approach, the client can do a simple Type-switch to determine, if the error is an X-Error created by xgb or if it is another error and choose to ignore it.

Ultimately it is of coure up to you and I also don't want to come along as a smartass or anything. I really think xgb is a very good library, creating the possibility to communicate with X via pure go - and using the xcb protocol descriptions is simply the right way to go. I just think that because it is such a nice library and thrives to be pure-go, it should provide an API that very well integrates into the language, follows its spirit and provides all the power of go ;)

[BurntSushi]

If the panic happens in a different goroutine, a recover is not possible, as far as I understood. In the Article of the goblog describing defer, panic and recover it says

Yup, you're right. That sucks.

Regarding the X-Errors vs. Connection-errors I agree with you, that X-Errors are a very common thing, but I disagree with you, that they should be ignored because of that. They are a lot like warnings, just because you can ignore them without immediately crashing your program, doesn't mean you should ignore them - they still tell you that there is some flaw in your program, some expectiaton not met or some calculation not right and you should fix them.

No no. I meant that X errors are very much depended on for the control flow of a client. Sometimes it is appropriate to completely and utterly ignore an X error (say, if you're fighting a race condition as the WM with a client when a window is unmanaged by using a side-effects-only unchecked request). But much of the time, the error is actually actionable. Here's one of many examples.

I was groaning because handling a connection error is different from handling a regular X error. This is not the typical case when writing Go code. Usually there's an error, and it gets tossed back to the caller. It's pretty rare to have to perform case analysis on an error. Adding a connection error into the mix requires analyzing an additional case. And since every X call can return an error, that's a lot of cases.

However, if it can be cornered into the event loop, then the client must handle it. Read on...

Regarding the complexity of the change: A very cursory reading of the source suggests, that it would suffice, to change some return values from the type Error to the builtin error-type and writing the Read-error to the Conn.eventChan of the current connection. Haven't tried this yet, but is there an obvious fault with that, which I am missing?

I like this on the surface. The idea of cornering connection errors into the event loop is something that I'd find acceptable. Namely, it restricts the areas where additional case analysis needs to be performed. It would be the client's responsibility to detect a connection error and do what it needs to do.

The complexity in this solution is bubbling the error out to all of the checked requests and requests waiting for replies, otherwise deadlock is almost guaranteed. It's late now, but I think that could work. I'll think on it some more.

I wonder if it's possible to get a write XOR read error. Or does one necessarily imply the other? Hmmm.

Ultimately it is of coure up to you and I also don't want to come along as a smartass or anything. I really think xgb is a very good library, creating the possibility to communicate with X via pure go - and using the xcb protocol descriptions is simply the right way to go. I just think that because it is such a nice library and thrives to be pure-go, it should provide an API that very well integrates into the language, follows its spirit and provides all the power of go ;)

Thanks :-)

Just remember, there are trade offs everywhere. The Go Way should not be accepted in every case. If we can make it work in XGB without much trouble, I'd obviously prefer it. But if it introduces an extra case into each error check, there's just no way I can subject myself to that in the name of idiomatic code. I don't need to be convinced that idiomatic code is good; I would need to be convinced that the advantages of idiomatic code outweigh the disadvantages. :-)

By the way, I'd consider XGB crashing without the possibility for recovery outside the scope of The Go Way. It's just plain rude in any language. :P