Remove tracking of unregistered connectors

[ahie]

Closes #8111, fixes #7748

---

This change is 

[denis-anisimov]

Needs rebase. There is a failing test.

[denis-anisimov]

Reviewed 15 of 15 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):
A several questions:

• I didn't get from the patch what causes the memory leak. As I understand the logic which should have removed the connectors from a map wasn't woking in some cases (when ?)
• Push test is completely removed. Shouldn't there be a replacement for it (testing a happy path may be) ?
• Is it possible to make a test for the patch ?

For the latter question: it's possible to test that reference to an object is GCed. The question is : can we make a test assuming that we can check the absence of an object (it has been GCed) ?

---

Comments from Reviewable

[Legioth]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

it's possible to test that reference to an object is GCed.

See CurrentInstanceTest.waitUntilGarbageCollected(WeakReference<?>)

---

Comments from Reviewable

[Artur-]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

I didn't get from the patch what causes the memory leak. As I understand the logic which should have removed the connectors from a map wasn't woking in some cases (when ?)

A connector is added to ConnectorTracker.syncIdToUnregisteredConnectorIds on component detach. It's cleaned when the browser does a request. If the browser does not do a request, it's never cleaned

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, Artur- (Artur) wrote…

I didn't get from the patch what causes the memory leak. As I understand the logic which should have removed the connectors from a map wasn't woking in some cases (when ?)

A connector is added to ConnectorTracker.syncIdToUnregisteredConnectorIds on component detach. It's cleaned when the browser does a request. If the browser does not do a request, it's never cleaned

A ConnectorTracker is 1:1 to UI, right ?
Each time when UI get a browser request the map is cleaned.
So the issue happens only when there are no anymore requests to the UI. But it that case the UI will be closed at some point. Can the clean up be done there instead (or why it doesn't work if it's already done) ?

---

Comments from Reviewable

[Artur-]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

A ConnectorTracker is 1:1 to UI, right ?
Each time when UI get a browser request the map is cleaned.
So the issue happens only when there are no anymore requests to the UI. But it that case the UI will be closed at some point. Can the clean up be done there instead (or why it doesn't work if it's already done) ?

Do you have any good reason for keeping the map?

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, Artur- (Artur) wrote…

Do you have any good reason for keeping the map?

Not really........ But.
The code looks fragile for me at the moment: you have to remember to implement the setParent() method properly.
The method can be completely overridden with some custom logic. But you have to remember to do the clean up.
It would be better if this happens automatically.....
That's the only reason.

---

Comments from Reviewable

[ahie]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

Not really........ But.
The code looks fragile for me at the moment: you have to remember to implement the setParent() method properly.
The method can be completely overridden with some custom logic. But you have to remember to do the clean up.
It would be better if this happens automatically.....
That's the only reason.

The push test was added to test the functionality of the acknowledge RPC (5b18c54) which has been removed in this patch. Since the map causing the memory leak is completely removed, there are no references to test with waitUntilGarbageCollected or similar. The removal of the push test could be reverted, it will now pass without the acknowledge messages, but I don't see any added value in doing so.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, ahie (Aleksi Hietanen) wrote…

The push test was added to test the functionality of the acknowledge RPC (5b18c54) which has been removed in this patch. Since the map causing the memory leak is completely removed, there are no references to test with waitUntilGarbageCollected or similar. The removal of the push test could be reverted, it will now pass without the acknowledge messages, but I don't see any added value in doing so.

OK. Got it, thanks.

What about the test for the patch ( that there are no memory leaks) ?

---

Comments from Reviewable

[Legioth]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

OK. Got it, thanks.

What about the test for the patch ( that there are no memory leaks) ?

The thing that could still be tested in the same way as waitUntilGarbageCollected would be that a component that has been detached is actually garbage collected.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, Legioth (Leif Åstrand) wrote…

The thing that could still be tested in the same way as waitUntilGarbageCollected would be that a component that has been detached is actually garbage collected.

That's what I meant.

---

Comments from Reviewable

[ahie]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

That's what I meant.

This patch doesn't effect the GC of components themselves, only a string identifier is stored in the map that has been removed.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):

Previously, ahie (Aleksi Hietanen) wrote…

This patch doesn't effect the GC of components themselves, only a string identifier is stored in the map that has been removed.

Right.........
It doesn't use a map anymore where the memory leak could appear.
So a memory leek is caused by the map where only integer numbers and strings are stored.....
Cool.

But.

There are changes in the patch that replaces the logic which has been implemented with the map.
This should be tested.
I'm not sure about testing it in UI (the consequence of calling markAsDirty() for an old parent) but at least there should be a unit test that checks that markAsDirty() is called when setParent()is called.

---

Comments from Reviewable

[ahie]

Review status: 15 of 17 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

Right.........
It doesn't use a map anymore where the memory leak could appear.
So a memory leek is caused by the map where only integer numbers and strings are stored.....
Cool.

But.

There are changes in the patch that replaces the logic which has been implemented with the map.
This should be tested.
I'm not sure about testing it in UI (the consequence of calling markAsDirty() for an old parent) but at least there should be a unit test that checks that markAsDirty() is called when setParent()is called.

Done.

---

Comments from Reviewable

[denis-anisimov]

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

server/src/test/java/com/vaadin/tests/server/abstractextension/AbstractExtensionSetParentTest.java, line 21 at r2 (raw file):

        extension.setParent(connector);
        extension.setParent(null);
        Mockito.verify(connector, Mockito.times(1)).markAsDirty();

JFYI : Mockito.times(1) shouldn't be needed. The verify() method checks this by default.
No need to correct. Just a note.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

a discussion (no related file):

Previously, ahie (Aleksi Hietanen) wrote…

Done.

@Artur- , please unblock.

---

Comments from Reviewable