PR: Close memory leaks of CodeEditor and ShellWidget #18781

impact27 · 2022-07-22T07:13:59Z

Description of Changes

Wrote at least one-line docstrings (for any new functions)
Added unit test(s) covering the changes (if testable)

Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

pep8speaks · 2022-07-22T07:14:06Z

Hello @impact27! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-28 21:49:56 UTC

ccordoba12 · 2022-07-22T13:55:42Z

Hey @impact27, this is great! Thanks a lot for taking the time to fix these memory leaks!

Could you move your branch to 5.x so we can include your PR in our next release? For that, please execute the following commands:

git checkout close_leaks
git rebase --onto 5.x master close_leaks
git push -f origin close_leaks

spyder/plugins/editor/plugin.py

impact27 · 2022-07-25T12:08:16Z

@ccordoba12 This is ready for review. The failures are unrelated to this PR

ccordoba12

Thanks @impact27 for your hard work on this one!

spyder/app/tests/test_mainwindow.py

ccordoba12 · 2022-07-25T14:32:15Z

spyder/app/tests/test_mainwindow.py

+    code_editor = main_window.editor.get_focus_widget()
+    # Show an error in the editor
+    code_editor.set_text("aaa")
+    del code_editor


Why do you need to delete this CodeEditor instance at this point?

Otherwise a reference is kept which leads to leaking a CodeEditor

I am checking the number of CodeEditor with a reference below, this would add one. Del only deletes the local ref, not the object itself (unless this is the last ref)

spyder/app/tests/test_mainwindow.py

ccordoba12 · 2022-07-25T14:36:00Z

spyder/plugins/completion/providers/languageserver/providers/document.py

@@ -309,7 +309,7 @@ def document_did_close(self, params):
                if id(codeeditor) == id(editor):
                    idx = i
                    break
-            if idx > 0:
+            if idx >= 0:


Why is it necessary to add the equal sign here? Just asking because I don't understand it.

I think this was an error in the code. If no CodeEdotor are found, the index should be -1, so you can check with index >=0 (and not >0 which leads to a leak)

spyder/plugins/editor/utils/decoration.py

spyder/plugins/editor/widgets/editor.py

spyder/plugins/ipythonconsole/widgets/client.py

spyder/plugins/ipythonconsole/widgets/main_widget.py

ccordoba12

Last comment for you @impact27, then this should be ready.

spyder/app/tests/test_mainwindow.py

spyder/config/main.py

more better

fix test simpler tests

close_more_clients Update main_widget.py _close_client cleanup close_client

fix test

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com> Update test_mainwindow.py Update main.py

ccordoba12

Thanks @impact27 for your hard work on this one!

Fixes #18764

ccordoba12 changed the base branch from master to 5.x July 22, 2022 13:49

ccordoba12 changed the base branch from 5.x to master July 22, 2022 13:50

ccordoba12 assigned impact27 Jul 22, 2022

ccordoba12 added this to the v5.3.3 milestone Jul 22, 2022

impact27 force-pushed the close_leaks branch from 7596b1b to 88f065b Compare July 22, 2022 16:47

ccordoba12 changed the base branch from master to 5.x July 22, 2022 17:47

ccordoba12 changed the base branch from 5.x to master July 22, 2022 17:47

impact27 force-pushed the close_leaks branch from 9ab93ba to 6b3b0a0 Compare July 23, 2022 06:44

impact27 changed the base branch from master to 5.x July 23, 2022 06:45

impact27 force-pushed the close_leaks branch 7 times, most recently from 7247f3c to d65dab1 Compare July 23, 2022 16:37

ccordoba12 reviewed Jul 23, 2022

View reviewed changes

spyder/plugins/editor/plugin.py Outdated Show resolved Hide resolved

impact27 force-pushed the close_leaks branch 2 times, most recently from 0749a64 to 4f07bda Compare July 24, 2022 12:41

ccordoba12 reviewed Jul 25, 2022

View reviewed changes

ccordoba12 reviewed Jul 26, 2022

View reviewed changes

spyder/app/tests/test_mainwindow.py Show resolved Hide resolved

spyder/config/main.py Outdated Show resolved Hide resolved

Quentin Peter added 6 commits July 28, 2022 23:44

unregister_shortcuts

98b5ac4

close_leaks

1712130

more better

kite_call_to_action

0c6f761

Better tests

c6e27f8

fix test simpler tests

weakref instance_response

9e4f05a

close

f24a6c2

close_more_clients Update main_widget.py _close_client cleanup close_client

Quentin Peter and others added 2 commits July 28, 2022 23:49

sig_time_label

df59eb5

fix test

Apply suggestions from code review

e3713ae

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com> Update test_mainwindow.py Update main.py

impact27 force-pushed the close_leaks branch from 4b04ae7 to e3713ae Compare July 28, 2022 21:49

ccordoba12 approved these changes Jul 28, 2022

View reviewed changes

ccordoba12 merged commit d51ee6c into spyder-ide:5.x Jul 28, 2022

ccordoba12 added a commit that referenced this pull request Jul 28, 2022

Merge from 5.x: PR #18781

24a1b41

Fixes #18764

ccordoba12 mentioned this pull request Jul 29, 2022

test_leaks is failing on master #18855

Closed

ccordoba12 mentioned this pull request Aug 17, 2022

PR: Restore the previous way of closing all clients (IPython console) #19085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Close memory leaks of CodeEditor and ShellWidget #18781

PR: Close memory leaks of CodeEditor and ShellWidget #18781

impact27 commented Jul 22, 2022 •

edited by ccordoba12

Loading

pep8speaks commented Jul 22, 2022 •

edited

Loading

ccordoba12 commented Jul 22, 2022

impact27 commented Jul 25, 2022

ccordoba12 left a comment

ccordoba12 Jul 25, 2022

impact27 Jul 25, 2022

impact27 Jul 25, 2022

ccordoba12 Jul 25, 2022 •

edited

Loading

impact27 Jul 25, 2022

ccordoba12 left a comment •

edited

Loading

ccordoba12 left a comment •

edited

Loading

PR: Close memory leaks of CodeEditor and ShellWidget #18781

PR: Close memory leaks of CodeEditor and ShellWidget #18781

Conversation

impact27 commented Jul 22, 2022 • edited by ccordoba12 Loading

Description of Changes

Issue(s) Resolved

Affirmation

pep8speaks commented Jul 22, 2022 • edited Loading

Comment last updated at 2022-07-28 21:49:56 UTC

ccordoba12 commented Jul 22, 2022

impact27 commented Jul 25, 2022

ccordoba12 left a comment

Choose a reason for hiding this comment

ccordoba12 Jul 25, 2022

Choose a reason for hiding this comment

impact27 Jul 25, 2022

Choose a reason for hiding this comment

impact27 Jul 25, 2022

Choose a reason for hiding this comment

ccordoba12 Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

impact27 Jul 25, 2022

Choose a reason for hiding this comment

ccordoba12 left a comment • edited Loading

Choose a reason for hiding this comment

ccordoba12 left a comment • edited Loading

Choose a reason for hiding this comment

impact27 commented Jul 22, 2022 •

edited by ccordoba12

Loading

pep8speaks commented Jul 22, 2022 •

edited

Loading

ccordoba12 Jul 25, 2022 •

edited

Loading

ccordoba12 left a comment •

edited

Loading

ccordoba12 left a comment •

edited

Loading