robotpy / robotpy-wpilib

Moved to https://github.com/robotpy/mostrobotpy
https://robotpy.github.io
Other
170 stars 60 forks source link

SmartDashboard::putData doesn't keep objects alive #685

Closed benjiboy50fonz closed 3 years ago

benjiboy50fonz commented 3 years ago

Hi devs, @virtuald ,

Managed to isolate this issue. It's rather odd; about two-thirds of the time it provides me with an access violation, and the other third provides me with this pointer issue. Nevertheless, the issue seems to be stemming from the creation of an object within what I assume is a static function, the putData() in SmartDashboard.

My OS is Windows 10, and my packages are up to date. My wpilib package is version 2021.2.2.0 and my robotpy-commands-v2 package is version 2021.2.2.0.

The error looks like so:

Traceback (most recent call last):
  File "C:\Users\Ben Bistline\AppData\Local\Programs\Python\Python39\lib\site-packages\wpilib\_impl\start.py", line 106, in start
    self.robot.startCompetition()
RuntimeError: bad_weak_ptr

Locals at innermost frame:

{ 'DriverStation': <class 'wpilib._wpilib.DriverStation'>,
  'NetworkTables': <_pyntcore._ntcore.NetworkTablesInstance object at 0x000001316C9FE670>,
  'hal': <module 'hal' from 'C:\\Users\\Ben Bistline\\AppData\\Local\\Programs\\Python\\Python39\...
  'isSimulation': True,
  'robot_cls': <class '__main__.Robot'>,
  'self': <wpilib._impl.start.RobotStarter object at 0x000001316EB0D280>,
  'wpilib': <module 'wpilib' from 'C:\\Users\\Ben Bistline\\AppData\\Local\\Programs\\Python\\Pyt...

Error at C:\Users\Ben Bistline\AppData\Local\Programs\Python\Python39\lib\site-packages\wpilib\_impl\start.py.106:start: Unhandled exception

Traceback (most recent call last):
  File "C:\Users\Ben Bistline\AppData\Local\Programs\Python\Python39\lib\site-packages\wpilib\_impl\start.py", line 106, in start
    self.robot.startCompetition()
  File "C:\Users\Ben Bistline\AppData\Local\Programs\Python\Python39\lib\site-packages\wpilib\_impl\start.py", line 106, in start
    self.robot.startCompetition()
RuntimeError: bad_weak_ptr

The code to replicate this issue is this:

#!/usr/bin/env python3

from commands2 import TimedCommandRobot, SequentialCommandGroup, WaitCommand
from wpilib._impl.main import run
from wpilib import RobotBase, SmartDashboard

import shutil, sys

def showCommand(cmd):
    """Display the given command on the dashboard."""

    name = cmd.getName()
    name.replace("/", "_")
    SmartDashboard.putData("Commands/%s" % name, cmd)

class TestCommandGroup(SequentialCommandGroup):
    def __init__(self):
        super().__init__()

        cmd = WaitCommand(1)

        self.addCommands(
            cmd,
        )

class Robot(TimedCommandRobot):
    """Implements a Command Based robot design"""

    def robotInit(self):
        """Set up everything we need for a working robot."""

        self.myCommand = TestCommandGroup()

        showCommand(TestCommandGroup()) # Ny constructing the object in the function call, it fails.

    def teleopInit(self):
        self.myCommand.schedule()

    def autonomousInit(self):
        pass

if __name__ == "__main__":
    if len(sys.argv) > 1 and sys.argv[1] == "deploy":
        shutil.rmtree("opkg_cache", ignore_errors=True)
        shutil.rmtree("pip_cache", ignore_errors=True)

    run(Robot)

Again, this code appears to be rather unstable and unpredictable.

Thanks!

auscompgeek commented 3 years ago

If I had to guess, I'd say SmartDashboard.putData is not keeping objects passed in alive. Since the Python code is not keeping a reference to the object, it gets freed, and once SmartDashboard.updateValues is called (by TimedRobot), you end up with an attempted use-after-free.

auscompgeek commented 3 years ago

I can't seem to get this to crash on Linux, might be because Windows is a bit stricter on how memory is used.

TheTripleV commented 3 years ago

The keepalive is commented out https://github.com/robotpy/robotpy-wpilib/blob/main/gen/SmartDashboard.yml#L27.

TheTripleV commented 3 years ago

If a reference to cmd isn't kept, it does crash for me on Windows.

virtuald commented 3 years ago

@benjiboy50fonz this isn't valid code. If you putData with a command that is immediately deleted... it's immediately deleted. What are you expecting showCommand to actually do here? Nothing is keeping the command alive, so even if it didn't crash it shouldn't show anything on NetworkTables.

benjiboy50fonz commented 3 years ago

@virtuald Oh I get it now. Thank you guys, and my apologies everyone.

TheTripleV commented 3 years ago

Lol. Weird use case but

from commands2 import TimedCommandRobot, InstantCommand
from wpilib._impl.main import run
from wpilib import SmartDashboard

class Robot(TimedCommandRobot):

    def robotInit(self):
        self.myCommand = InstantCommand(lambda: print("hello"))
        # SmartDashboard.putData("mycommand", self.myCommand)     # WORKS
        SmartDashboard.putData("mycommand", InstantCommand(lambda: print("hello"))) # DOESNT WORK

    def teleopInit(self) -> None:
        SmartDashboard.getData("mycommand").schedule()

if __name__ == "__main__":
    run(Robot)

I guess teams may run into this if they put a bunch of autonomous commands into networktables and use shuffleboard as an auto selector. But, idk if anyone should/will do this.

auscompgeek commented 3 years ago

use shuffleboard as an auto selector

Doesn't SendableChooser keep references to its objects?

virtuald commented 3 years ago

Yes, SendableChooser contains py::object, which will ensure it doesn't remove references.

virtuald commented 3 years ago

I think the reason I hadn't used keepalive for putData was because of the potential to just keep putting the same thing over and over again and leaking like crazy. I think the appropriate way to deal with this is to have a hidden dictionary that putData stores the object in. That way, if you keep creating new objects and putting them on the same key they'll stay alive and the old objects will be deleted.

Still won't likely lead to anything useful, but will at least enable a putData/getData combination to work as expected.