eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

unlimited searchs can accidently hang/crash the system #97 #184

Closed r6squeegee closed 1 year ago

r6squeegee commented 1 year ago

this is to help address https://github.com/eclipse-platform/eclipse.platform.ui/issues/872

I should note though, I'm unsure of how the junit tests work here, as they don't resemble junit tests I'm familiar with and looking at the existing ones... I'm not so sure if or where the other preferences are tested. As a result I've tested this manually but have not added any junit tests.

I also have no idea how the translations texts works here, I found the english text and added those but that's it.

mickaelistria commented 1 year ago

I think it's important that we still make the search capable of showing all matches. Would it be possible, when such "max" is reached, to show a popup asking user whether to abort the search or to continue it? Or even better, when the search is incomplete, there should be some visible warning on the Search result view telling something like "Search was aborted after finding many (2000) results. Double-click to complete full search". I can imagine that if I search for int in my workspace, I will easily get beyond 2k matches, and may still be interested in seeing the result just to count the number of occurrences in the projects.

jukzi commented 1 year ago

it's just impossible to find all results when searching for "a" in a big workspace. However a hint that search limit was reached would be nice for example here: image example shows also that 100_000 results are still no issue.

r6squeegee commented 1 year ago

I've set the default for this to disabled, so this in theory should not affect anyone unless they explicitly turn it on. With it turned off it will skip all calls to getMatchCount() so this should help with that regression issue.

r6squeegee commented 1 year ago

oh and to further continue the how quickly it crashes/hangs conversation...

cpu : i5-8250u jvm ram : the default (-Xms256m -Xmx2048m) ssd : yes...

2 projects in my workspace

scenario :

jukzi commented 1 year ago

@r6squeegee please (always) squash all commits of a PR to a single commit. Exceptions being version counter increase.

jukzi commented 1 year ago
  • time till eclipse completely hangs / unresponsive ~0.5 seconds

@r6squeegee please investigate what is eating all the memory. maybe there is some sort of leak for regexp?

iloveeclipse commented 1 year ago

Also please rebase your PR on top of master (no merge commits please)

r6squeegee commented 1 year ago
  • time till eclipse completely hangs / unresponsive ~0.5 seconds

@r6squeegee please investigate what is eating all the memory. maybe there is some sort of leak for regexp?

interesting, with the latest checkout (that I just rebased upon) it no longer happens (was still happening on my original checkout from a short while ago). I guess some one solved it some where.

(It wasn't memory on the prior checkout btw, I checked on jconsole, the memory and cpu were fine, it just "hung")

mickaelistria commented 1 year ago

So let's close it as eclipse-platform/eclipse.platform.text#189 seems to have fixed it. The track followed here was interesting initially but quickly lost focus on the actual issue. Usually, the best approach in case of a hang is not to reduce the feature, but really to investigate what is blocked and why. Calling jstack a few times and looking at what happens in the main thread tells what's blocked.

iloveeclipse commented 1 year ago

Why not have a limit, if it can be iseful for someone?

mickaelistria commented 1 year ago

OK, I've re-read eclipse-platform/eclipse.platform.ui#872 and indeed, a limit can be useful independently of UI. So let's reopen.

jukzi commented 1 year ago

@r6squeegee do you plan to continue to work on a limit?

r6squeegee commented 1 year ago

@r6squeegee do you plan to continue to work on a limit?

last I recall, I'd implemented all the requested changes, and it was working, and I'd rebased and squashed. I think it was ready. Was there a requested change I missed?

jukzi commented 1 year ago

@iloveeclipse still changes requested?

iloveeclipse commented 1 year ago

@iloveeclipse still changes requested?

Will check next week. Going into longer weekend now.

laeubi commented 1 year ago

This repository was merged with https://github.com/eclipse-platform/eclipse.platform.ui if your pull-request is still relevant please rebase the code and push it to the new repository.