search

apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.63k stars 840 forks source link

Cleanup ContextAction and fix TerminalAction behavior #7451

Closed matthiasblaesing closed 1 month ago

matthiasblaesing commented 3 months ago

The core problem reported by #4199 is most probably introduced by 562cfade954ea27308de01738813d050d1b57ce1. That change made the action stateful and in turn the Performer must follow that state ties. The concrete case are ContextAwareAction instances, which are created per lookup.

See the commits:

for details.

While testing this change two further problems were observed:

Closes: #4199

matthiasblaesing commented 3 months ago

@eirikbakke could you please have a look at this and see if this fixes the issue in your application? @neilcsmith-net your comment:

I say partially fix the title issue, as there seems to be a secondary issue with the context not changing to an unfocused tab when invoking the menu on it?

should be fixed by 0fd9f03

eirikbakke commented 3 months ago

Thank you for your work on this! I'll see if I can reproduce NETBEANS-4754 without the patch (and without my old hacky patch), and then see if it disappears with this PR.

matthiasblaesing commented 3 months ago

@eirikbakke I added a test, that uses an ActionListener as basis in addition to the ContextAwareAction test, to cover that case also. Of course this still needs to be validated in "in the wild". Thank you for that.

eirikbakke commented 2 months ago

I'll have to run with my old patch removed for a while before testing this one, as I'm waiting for the NETBEANS-4754 bug to randomly reappear on the latest NetBeans version. Hopefully I'll find a reliable way to reproduce it again... and then I can test if this PR fixes it.

matthiasblaesing commented 2 months ago

Is there any benefit to keeping the delegate in a weak reference now there's a 1-to-1 mapping of context action to performer? Having any GC dependent code requires thought and care, and if we can get rid of it now and not have GC affected behaviour, then all the better.

Consider a normal ContextAwareAction, this means a ContextAwareAction without state. In that case the IDE can spin up a huge number of ContextAction/Performer combinations. The instDelegate is only created when the action is actually being invoked. If that action now is "big" (for example because their target is huge), having it in a WeakReference allows it to be removed.

neilcsmith-net commented 2 months ago

Consider a normal ContextAwareAction, this means a ContextAwareAction without state. In that case the IDE can spin up a huge number of ContextAction/Performer combinations. The instDelegate is only created when the action is actually being invoked. If that action now is "big" (for example because their target is huge),

Well, "big" could mean a lot, including "big" to compute. But what do you mean by "target"? If that's in the lookup context then surely it's held by a strong reference anyway? Isn't the performer chain meant to invalidate and clear anyway when that reference goes from the context?

I'm happy with the change you have, and it's probably the safer option. I retain a concern we're potentially masking issues with GC effects.

sdedic commented 2 months ago

@matthiasblaesing is it possible to give me some time (~ 1-2 weeks) so I can try to supply more tests based on IdealGraphVisualizer stateful graph actions, which were one of the reasons for introducing better action state ?

matthiasblaesing commented 2 months ago

@matthiasblaesing is it possible to give me some time (~ 1-2 weeks) so I can try to supply more tests based on IdealGraphVisualizer stateful graph actions, which were one of the reasons for introducing better action state ?

Sure - no problem.

eirikbakke commented 2 months ago

The changes look OK to me, reviewing each commit by itself. But the logic is complicated enough that I feel like correctness can only be ensured by manually testing it for some time.

(I'm still waiting for https://issues.apache.org/jira/browse/NETBEANS-4754 to reappear without this PR or my earlier patch first. I have not yet been able to reproduce it... but I remember it was dependent on timing, memory conditions, and the phase of the moon...)

neilcsmith-net commented 2 months ago

(I'm still waiting for https://issues.apache.org/jira/browse/NETBEANS-4754 to reappear without this PR or my earlier patch first. I have not yet been able to reproduce it... but I remember it was dependent on timing, memory conditions, and the phase of the moon...)

That bothers me somewhat given how reliably reproducible the bug with the terminal action is. Be good to know if you get to the bottom of the steps that trigger it.

eirikbakke commented 2 months ago

Yeah, I'm not sure why I can't reproduce it anymore. I had reproduction steps written up from 2020, and I tried building the same version of NetBeans and my platform app, and use the same Java version and laptop. Maybe I made a mistake somewhere... I'll have to try again.

matthiasblaesing commented 2 months ago

You need to have a situation where the same action is invoked with different lookups. For the TerminalAction this is trivial as the actions are created everytime a popup menu is created for the terminals. Thus it is nearly instantatious.

https://github.com/apache/netbeans/blob/b332b296700219ecd1d6ee6255124f3486171af3/ide/terminal.nb/src/org/netbeans/modules/terminal/ioprovider/Terminal.java#L1038-L1097

with

https://github.com/apache/netbeans/blob/b332b296700219ecd1d6ee6255124f3486171af3/platform/openide.util.ui/src/org/openide/util/Utilities.java#L1819-L1899

matthiasblaesing commented 2 months ago

@eirikbakke @sdedic any updates on this?

eirikbakke commented 2 months ago

@matthiasblaesing I have not been able to make the original NETBEANS-4754 bug resurface yet. I was hoping it would pop up randomly after I removed my old patch, but I will need to go back to some more targeted attempts at reproducing it (including finding the same old Java version etc.).

matthiasblaesing commented 1 month ago

Having heared no objections, I plan to merge this early next week. If anyone wants to object, please do so now.

neilcsmith-net commented 1 month ago

@matthiasblaesing I'd merge earlier if for NB23 given freeze was meant to be this week???

eirikbakke commented 1 month ago

I'm switch my working IDE and platform app to running this patch now, for testing purposes. I don't mind the patch being merged; even if something like NETBEANS-4754 shows up again, it'll be easier to debug again with the cleaned-up code.

matthiasblaesing commented 1 month ago

@neilcsmith-net thanks for the reminder. Ok, I'll give it another 24hours and merge then.

matthiasblaesing commented 1 month ago

I pushed the missing squash and will merge once green.