fix: fix memory leaks - #406
Conversation
we always should return a result or else cordova is leaking callbacks on the js side
| try (FileOutputStream out = new FileOutputStream(installation)) { | ||
| out.write(bytes); | ||
| logger.info("Successfully captured envelope."); | ||
| callbackContext.success(); |
There was a problem hiding this comment.
Why not move it to line 156 so it will always invoke callbackContext.success(); for all the cases?
There was a problem hiding this comment.
just to be sure, you want me to also replace all success plugin results (not in a try block, to avoid double call in case of catched errors) with the invokation at line 156 ?
| callbackContext.success(); | ||
| } catch (Exception e) { | ||
| logger.info("Error writing envelope."); | ||
| callbackContext.error("Error writing envelope."); |
There was a problem hiding this comment.
might also be a good idea to return false here
There was a problem hiding this comment.
I'm not sure, because as I understand it, the execute method is only supposed to return false if the requested action is not found.
(because when returning false cordova is sending its own plugin result)
|
Thank you @Athorcis for spotting this issue! I left a small suggestion on the java code but other than that it looks good! |
|
@lucas-zimerman I have questions about the suggestions |
lucas-zimerman
left a comment
lucas-zimerman
left a comment
There was a problem hiding this comment.
After a further review, I conclude my comments won't fix anything else, so with that, thank you for the contribution!
2 participants
we always should return a result or else cordova is leaking callbacks on the js side
it become visible with numerous calls to addBreadcrumb