search

pimoroni / vl53l1x-python

Python library for the VL53L1X Laser Ranger
https://shop.pimoroni.com/products/vl53l1x-breakout
94 stars 53 forks source link

fixed a resource leak #43

Closed ng-labo closed 4 years ago

ng-labo commented 4 years ago

I suggest a small fix. I've found a file descriptor leak which stays a device opened. I believe this is not critical usually and I know a work around, but in some case this gets reached 'Too many open files:'.

Given bus-number 1 for example as current situation, /dev/i2c-1 is opened in VL53L1X constructor(init()) , and same one is opened in VL53L1X.open() again. First one's reference is lost and unclosable.

According to package smbus2, smbus2.SMBus() with bus-number opens /dev/i2c-X in itself or not open it without bus-number. Considering to let the pair of VL53L1X.open() and VL53L1X.close() work effectively, at the end of constructor /dev/i2c-X should be closed once.

Gadgetoid commented 4 years ago

You're right. Thank you!

Ideally, the self._i2c.close() should be in a finally: clause so that it still cleans up the file handle in the (rare) event someone catches the RuntimeError.

Being even more pedantic, in this case the if statement should not be inside the try/except at all:

    self._i2c = SMBus()
    if tca9548a_num == 255:
        try:
            self._i2c.open(bus=self._i2c_bus)
            self._i2c.read_byte_data(self.i2c_address, 0x00)
        except IOError:
            raise RuntimeError("VL53L1X not found on adddress: {:02x}".format(self.i2c_address))
        finally:
            self._i2c.close()
ng-labo commented 4 years ago

I appreciate to point out to my request. It makes sense to put 'close' inside finally.

Fortunately my VL53L1X device (on ras-pi zero) has been working fine without error in a some long time. I expect this comment help me to avoid same situation when I2C move into a sudden error state.

Gadgetoid commented 4 years ago

Thank you for making the suggested change! Much appreciated.