Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[sonic_platform_base] Reallocate empty lists in the constructor of the base class #65

Merged
merged 1 commit into from
Nov 13, 2019

Conversation

ds952811
Copy link
Contributor

… the base class

When we use a mutable object as a default value in the base class, a value that can be modified in place in the subclasses, such as the empty lists of
the chassis base class, and it is most likely unwanted.

And hence, this commit will try to address this issue by allocating new empty
list objects at the constructor of the base class, and the subclass should
rely on the constructor of superclass for this.

As a result, it simplifies the source code, and reduce the maintenance efforts

Signed-off-by: Dante (Kuo-Jung) Su dante.su@broadcom.com

… the base class

When we use a mutable object as a default value in the base class, a value that can be modified in place in the subclasses, such as the empty lists of
the chassis base class, and it is most likely unwanted.

And hence, this commit will try to address this issue by allocating new empty
list objects at the constructor of the base class, and the subclass should
rely on the constructor of superclass for this.

As a result, it simplifies the source code, and reduce the maintenance efforts

Signed-off-by: Dante (Kuo-Jung) Su <dante.su@broadcom.com>
@ds952811
Copy link
Contributor Author

Here is an example to help you understand what's exactly the problem we try to address.
Please note we have 2 different results of FAN number in the following test, and it's because the LIST is a mutable object, and it's modified in place in the subclasses

root@sonic:~# cat test.py
#!/usr/bin/env python

import sonic_platform.platform

chassis = sonic_platform.platform.Platform().get_chassis()
if chassis is None:
print("Error getting chassis!")
sys.exit(1)

print("FAN number: {}".format(chassis.get_num_fans()))

chassis = sonic_platform.platform.Platform().get_chassis()
if chassis is None:
print("Error getting chassis!")
sys.exit(1)

print("FAN number: {}".format(chassis.get_num_fans()))

root@sonic:# ./test.py
FAN number: 6
FAN number: 12
root@sonic:
#

@jleveque
Copy link
Contributor

So just to confirm, you're claiming that with the current code, after the second call to sonic_platform.platform.Platform().get_chassis() you will have double the number of devices (int this case, fans) because you have populated the global lists twice?

@ds952811
Copy link
Contributor Author

So just to confirm, you're claiming that with the current code, after the second call to sonic_platform.platform.Platform().get_chassis() you will have double the number of devices (int this case, fans) because you have populated the global lists twice?

Yes, as the fan list of the base class will be modified in place, so the number of devices will be double at 2nd call, and triple at 3rd call.

The same issues apply to all the LIST objects in the base classes (As they're mutable objects)

@ds952811
Copy link
Contributor Author

Here is a good article to help you have a better understanding of this issue
https://blog.florimond.dev/python-mutable-defaults-are-the-source-of-all-evil

@jleveque jleveque changed the title [sonic_platform_base] reallocate the empty LIST at the constructor of… [sonic_platform_base] Reallocate empty lists in the constructor of the base class Nov 13, 2019
@jleveque jleveque merged commit 9bae794 into sonic-net:master Nov 13, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants