coding-buddies-discord / cb-bots

4 stars 3 forks source link

Fix/memory leak #53

Closed waream2 closed 1 year ago

waream2 commented 1 year ago

This PR fixes the memory leak associated with Points reporting.

waream2 commented 1 year ago

I think that I'm may looking to behavior that I'm not used to, so correct me if I'm wrong: In the file imgFromHtmlGenerator.js there's a try but not a catch. I'm guessing that since there's no catch there the error will be lift up and go to the catch at line 41 of reportChannelPoints.js.

This is correct. There is no catch here because there's no intention to handle the error here. It will simply just be returned and that at point we catch it and handle it in reportChannelPoints. I chose to go this route because we don't really have true error handling in the bot. So for that reason, I'd rather just handle the error and return the response in reportChannelPoints because that module has access to the interaction and can respond.

Not married to this, if you'd rather handle it in imgFromHtmlGenerator I'd love to hear the case for it.

ETA: the only reason we need a try here is so that we can have a finally to clean up and kill the browser instance.

cubiquitous commented 1 year ago

I don't see any problem there. I personally find not putting a catch in imgFromHtmlGenerator, even if it's just to lift the error, a little weird. If I had to think for a few minutes how that would work it could also mean that others will have hard time understanding. The real problem here is that the process of lifting up the error is implicitly and therefore not so clear.

I would personally prefer to have the error lifting explicitly, but since it can easily argued that this is a matter of style, I don't see any reason why not merge this.

waream2 commented 1 year ago

I agree. I think once we have a proper error handling strategy, we will rip this out and rethink it.

Thanks for the thoughts, I'm merging now.