Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Brain: avoid retaining dangerous robot/brain references #1496

Merged
merged 1 commit into from
Mar 6, 2019

Conversation

mistydemeo
Copy link
Contributor

It appears that Hubot is accidentally retaining some references to robot and brain in objects, leading to unserializable circular references. This fixes the issue by

a) Ensuring temporary assignment of robot to objects is removed, and
b) Tucking robot inside a getter function in Brain, instead of assigning it as a variable, like in the User object.

Copy link
Member

@technicalpickles technicalpickles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Are there any tests you can think of that would need to be added? Or any other way to capture the notion that it's real bad to persist an instance of the Robot in the brain?

@mistydemeo
Copy link
Contributor Author

Looks good. Are there any tests you can think of that would need to be added?

Let me see if there's a decent npm module I can use to create tests for this. The problem is detecting the cyclic structure!

@mistydemeo
Copy link
Contributor Author

Updated to add tests using the is-circular module.

@mistydemeo mistydemeo merged commit c1c2611 into hubotio:master Mar 6, 2019
@mistydemeo mistydemeo deleted the fix_circular_reference branch March 6, 2019 17:11
@hubotbot
Copy link

hubotbot commented Mar 6, 2019

🎉 This PR is included in version 3.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants