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

modules: always safely set up/clear memory #1569

Merged
merged 5 commits into from
Jun 16, 2019

Conversation

HumorBaby
Copy link
Contributor

Modules will safely initialize (ensure key does not exist in memory already) in setup() and clean up (remove module-specific keys from memory) in shutdown(), where necessary.

@HumorBaby
Copy link
Contributor Author

HumorBaby commented Apr 24, 2019

Thinking more about the word "safely", I am brought to the question:

Wouldn't it be safer to raise something in setup() if a key is already present (where applicable)? Presumably shutdown() should remove keys, and a fresh load should not have the keys already; thus, the only time a key already exists would be an unwanted collision...

@dgw
Copy link
Member

dgw commented Apr 24, 2019

It depends. Sometimes it's OK to leave stuff in bot.memory so it can persist across module reloads (since we actually run shutdown() on unload now, IIRC). Things like the short-URL cache from url.py come to mind, where we don't necessarily want to have the bot hit the API again if it sees the same URL before & after reload. Basically, I think that just blanket-adding all of these might not be the best approach.

Let's see if I can come up with pros/cons for each of these.

@HumorBaby
Copy link
Contributor Author

HumorBaby commented Apr 24, 2019 via email

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Here are some evaluations of whether it makes sense to clear each memory key, as promised (though it took a bit longer than expected).

sopel/modules/find.py Show resolved Hide resolved
sopel/modules/help.py Outdated Show resolved Hide resolved
sopel/modules/ipython.py Outdated Show resolved Hide resolved
sopel/modules/safety.py Show resolved Hide resolved
sopel/modules/tell.py Outdated Show resolved Hide resolved
sopel/modules/tell.py Show resolved Hide resolved
sopel/modules/uptime.py Outdated Show resolved Hide resolved
sopel/modules/url.py Show resolved Hide resolved
@HumorBaby HumorBaby force-pushed the refactor-memory-safety branch from 9dd4aa0 to d3e1cc5 Compare April 25, 2019 11:54
@dgw dgw added the Tweak label Apr 25, 2019
@dgw dgw added this to the 7.0.0 milestone Apr 25, 2019
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

This brought up a new issue (which I'll open shortly), but I believe everything to be done here has been.

sopel/modules/url.py Show resolved Hide resolved
sopel/modules/safety.py Show resolved Hide resolved
@HumorBaby HumorBaby force-pushed the refactor-memory-safety branch from d3e1cc5 to 8474142 Compare April 25, 2019 18:09
@dgw dgw merged commit 09f4108 into sopel-irc:master Jun 16, 2019
@HumorBaby HumorBaby deleted the refactor-memory-safety branch August 28, 2019 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants