godbus / dbus

Native Go bindings for D-Bus
BSD 2-Clause "Simplified" License
981 stars 225 forks source link

Errors with a new connection if previous was closed #144

Closed luc-lynx closed 3 years ago

luc-lynx commented 5 years ago

In my code I use the following pattern

conn, err := dbus.SystemBus()
if err != nil {
    return nil, err
}
defer conn.Close()

And in the first function it works great, but the second function fails with error "dbus: connection closed by user". If I remove defer conn.Close() the both functions work great.

crashPopeye commented 5 years ago

The SystemBus() method actually return a singleton, if you close it it won't be available anymore. If you want to close your connection or need several connections you should use private connection, you also need to perform the DBus Handshake yourself.

conn, err := dbus.SystemBusPrivate()
if err != nil {
  return nil, err
}
if err := conn.Auth(nil); err != nil {
  conn.Close()
  return nil, err
}

if err := conn.Hello(); err != nil {
  conn.Close()
  return nil, err
}

This should works.

bminer commented 5 years ago

It would be really nice if there was a way to detect when the connections are closed. That way, you could, in theory, set the systemBus connection to nil and return a new singleton if it was closed and useless anyway.

Even when using SystemBusPrivate(), how can you detect when the connection is closed (i.e. closed manually, a decode error occurred, etc.)? Do you have to listen for the ErrClosed error for all methods? Can the Conn object expose a Connected property or method?

jsouthworth commented 5 years ago

@bminer it may be possible to do this when we are notified of the closed connection. We can at the very least restart the singleton instances when the conn.Close() method has been called (this is used internally when we detect a case that will result in termination as well). The handler implementations already track if the connection was closed. This won't be perfect but will handle the simple restart cases put forth in this issue. There are other ways that this can be handled (never allow closure of the shared instances, refcount the shared instances, etc.)

I'm open to accepting a patch for that if someone wants to do the work on it.

bminer commented 5 years ago

@jsouthworth - Thanks for your response. I think auto-reconnect would be a nice feature, but even just being able to detect a closed connection (without having to poll and check for an error) would be nice. How much effort would it be to add a "on closed" callback? Or even a simple IsConnected() method to the Conn instance?

EDIT: Perhaps Connected() is a better name than IsConnected()?

andrew-hoff commented 5 years ago

I actually would appreciate this feature as well. I think it would make the most sense to have the IsConnected() method that @bminer suggested as well as a step in SessionBus()/SystemBus() that creates a new singleton instance if the existing one is closed. That way, we aren't creating new instances when users explicitly call Close(), but we also give them the ability to create new instances after closing.

I can write up a pull request when I get some time :)

wzshiming commented 3 years ago

Hi, Is there any update to this issue?

jsouthworth commented 3 years ago

As I mentioned before. I’m not opposed to this addition if someone is willing to do the work on it.

wzshiming commented 3 years ago

@jsouthworth Hi, I sent a PR to patch. #232