-
Notifications
You must be signed in to change notification settings - Fork 27.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MaxListenersExceededWarning: Possible EventEmitter memory leak detected #51597
Comments
I think this is from the It would be good to remove that from the search service if we can. |
I added this listener to make sure ripgrep is killed when the search process goes down after reports of hanging |
If you type with / first, then it skips the cache, because it has to check the filesystem for every keypress. If that file doesn't exist then it runs rg. |
I would just remove that feature from search. |
Doing it all on the search side would be more efficient than doing part of it in the Maybe the check for an absolute file could be moved before the cache, so the cache can then be used if there was not hit. |
@chrmarti maybe more efficient, but does it make sense that the search service behaves like that? I think its contract is to search in the scope of the workspace and all opened files but not for the file path if it happens to be a file on disk and it is passed into the service. Besides, what does it mean for remote scenarios that a user puts in a remote file path, would we then also ask each remote file provider if the file exists? |
I agree that this makes more sense to handle outside of the search service. Opened a PR. |
Fix #51597 - move searching with absolute path queries out of search service and into openFileHandler
Steps:
@roblourens @chrmarti could this possibly come from the feature were we try to stat the path if it is absolute? Because I had to fix #42726 I lifted this check into the
openFileHandler
, so I would not need this kind of functionality anymore in the search service. I think we should probably remove it from there as it imho only makes sense in the context of quick open?The text was updated successfully, but these errors were encountered: