Skip to content
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

Ensure VB parser is disposed #76636

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Ensure VB parser is disposed #76636

merged 1 commit into from
Jan 8, 2025

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Jan 6, 2025

Noticed then when I was trying out adding a pooled object to the parser and that the dispose wasn't always being called to release the object back to the pool. I'm still investigating whether I want to add the pooled data to the parser, but minimially, the existing dispose method should be called.

Noticed then when I was trying out adding a pooled object to the parser and that the dispose wasn't always being called to release the object back to the pool. I'm still investigating whether I want to add the pooled data to the parser, but minimially, the existing dispose method should be called.
@ToddGrun ToddGrun requested a review from a team as a code owner January 6, 2025 16:09
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 6, 2025
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Scanner instance is only disposed when it is created by the Parser. When it is explicitly passed as an argument to the Parser then it is not disposed. This change seems to have no functional impact that I can see. Am I missing something?

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Jan 6, 2025

The Scanner instance is only disposed when it is created by the Parser. When it is explicitly passed as an argument to the Parser then it is not disposed. This change seems to have no functional impact that I can see. Am I missing something?

As mentioned in the PR description, this became visible to me as I was locally trying out adding an item to the VB Parser that needed to be cleaned up during dispose, regardless of whether the parser needed to dispose the scanner. In those cases, the missing Dispose calls were problematic. So yes, this PR shouldn't have any effect on current code, but it makes adding code that needs the Dispose easier and less error prone.

If you prefer, I can not make this a separate PR and only move forward if the change I'm investigating that needs the Dispose ends up being useful, but I considered this a good change even outside that context as it took me a bit to figure out why my objects were leaking in that the Dispose isn't called from all construction paths.

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Jan 7, 2025

@jaredpar -- Did I answer your question? Would you prefer for me to abandon this PR and just include it if I create a PR that adds something to the VB parser that needs to be disposed?

@jaredpar
Copy link
Member

jaredpar commented Jan 7, 2025

Want to make sure the current pattern wasn't a deliberate choice (@AlekseyTs)

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Jan 8, 2025

I've tested locally the disposable changes I'd like to make to the parser, and I'd like to get some speedometer test measurements.

@AlekseyTs -- was the old pattern intentional to avoid adding disposable objects into the parser except for the scanner?

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 1)

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Jan 8, 2025

@dotnet/roslyn-compiler for 2nd review

@ToddGrun ToddGrun merged commit df75ee9 into main Jan 8, 2025
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants