Improve handling of concurrent loads for MSBuild sub-projects #103

tintoy · 2024-03-29T04:25:37Z

Relates to #100, but it turns out the actual issue is related to concurrent loads of sub-projects, not overridden properties.

…SBuild property defined and then redefined in the same project file #100

#100

tintoy · 2024-03-29T04:26:25Z

@DoctorKrolic , yeah, sorry, got a bit carried away and added more doc comments 😂

DoctorKrolic · 2024-03-29T09:09:19Z

src/LanguageServer.Engine/Documents/MasterProjectDocument.cs

@@ -67,15 +68,21 @@ protected override void Dispose(bool disposing)
        /// <summary>
        ///     Add a sub-project.
        /// </summary>
-        /// <param name="subProjectDocument">
+        /// <param name="documentUri">
        ///     The sub-project.


This is why I don't like oversaturation of doc comments. This one is now obsolete and needs updating while IMO Uri documentUri is descriptive enough on its own and don't need additional commenting. Please just update the doc comment here, if we want to get rid of unnecessary noice we should do it in 1 pass as part of #91

Yeah fair call, I’ll update it to match for now but we can sort it out properly later (e.g. #91).

DoctorKrolic · 2024-03-29T09:12:11Z

src/LanguageServer.Engine/Documents/MasterProjectDocument.cs

@@ -22,7 +23,7 @@ public class MasterProjectDocument
        /// <summary>
        ///     Sub-projects (if any).
        /// </summary>
-        readonly Dictionary<Uri, SubProjectDocument> _subProjects = new Dictionary<Uri, SubProjectDocument>();
+        readonly ConcurrentDictionary<Uri, SubProjectDocument> _subProjects = new ConcurrentDictionary<Uri, SubProjectDocument>();


I don't remember if I asked you previously: what is your opinion on new() syntax? Using it in places like this can reduce the amount of changes in case like this one, but there are also perfectly legit arguments against new() syntax as well

I’m ok with either var or new() as long as the type name appears somewhere on the line. Will update!

DoctorKrolic · 2024-03-29T09:15:14Z

src/LanguageServer.Engine/Documents/Workspace.cs

-                    MasterProject = new MasterProjectDocument(this, documentUri, Log);
-                    return MasterProject;
-                }
+                    return MasterProject = new MasterProjectDocument(this, documentUri, Log);


Please don't use assignment expressions like this. If I remember correctly, we already had problems with this before when I accidentally removed such assignment without noticing

Fair enough; I don’t feel particularly strongly about their usage but if they’re already causing problems I’m happy to remove it.

DoctorKrolic · 2024-03-29T09:22:38Z

test/LanguageServer.Engine.Tests/MSBuildObjectLocatorTests.cs

+        /// <summary>
+        ///     The project collection for any projects loaded by the current test.
+        /// </summary>
+        ProjectCollection _projectCollection;


I am very uncomfortable having field which represents per-test data. Yes, it happens that XUnit runs tests in the same collection sequentially, but this is still a recipe for concurrency disaster if it ever changes or we annotate different tests in the file to be in different collections (that is possible, right?) and so on. You should create project collection in each test and use using statement, so it can be disposed properly. That would also remove the need to care about nulls, implementing IDisposable and so on

This was meant to be the first phase of producing base classes for test suites that work with XML, projects, etc (not that I’d indicated as such but it’s what I had in mind). I’d rather not have verbose setup code for each test (especially if they are the same) but I’ll have a think about whether we can make a little wrapper for it that can be used as you describe (I just want to avoid the situation where each test has to get the setup exactly right and it becomes hard to determine whether 2 tests are really creating the same configuration, besides the bits that are actually mean to be different from test to test)…

I am very uncomfortable having field which represents per-test data

As it turns out, xUnit creates a new instance of the test class per test:

xUnit.net creates a new instance of the test class for every test that is run

so using fields like this seems to be fine.

so using fields like this seems to be fine.

Yes indeed, that would work. But from pure design perspective having fields for per-test data should be avoided unless it is absolutely needed. Because now you depend on XUnit behavior and make everyone else contributing to actively remember that fact to be sure, that the field's value won't change during each test case run. Given, that this PR is merged, I think it is no longer relevant, but I would urge to refrain from this pattern in the future.

test/LanguageServer.Engine.Tests/MSBuildObjectLocatorTests.cs

test/LanguageServer.Engine.Tests/TestProject.cs

test/LanguageServer.Engine.Tests/TestProjects/RedefineProperty.SameFile.csproj

src/LanguageServer.Engine/Documents/MasterProjectDocument.cs

src/LanguageServer.Engine/Documents/Workspace.cs

#100 #103

DoctorKrolic · 2024-04-02T14:21:34Z

@tintoy It's a good practice to re-request review after your are done with previous feedback unless a hot fix is needed and so on. Your desire to release 0.6.3 feels rushed to me. Was that exact bug that much urgent to be fixed? Why you even consider it fully fixed (the PR says "related to", not "fixes" or "closes")? Why you missed versions 0.6.1 and 0.6.2 and released it as 0.6.3? I've seen release and tag 0.6.1 created, but they were on the same commit as 0.6.0, so I guess this was some automatically created stuff, that was actually unwanted, thus I deleted them. Also you didn't include other changes, that were introduced in this version, e.g. tintoy/msbuild-project-tools-vscode#145

tintoy · 2024-04-02T18:23:32Z

Hi. You’re right - I’ve been sick for the last 2 weeks and have been struggling to find time to get anything done outside of work and sleeping so I really did want to get something out (rather than just repeatedly rescheduling each task for the following day). I can see that it would be frustrating to have a collaborator publish changes without sufficient consultation.

Having said that:

I don’t think I’ve ever seen either of us ask for a second review after addressing PR feedback. I’m not sure what the point would be but am open to discussing it.
Yes, not every release version is public; some are “internal” and necessitated purely by the tooling (e.g. NPM’s insistence on failing if the package version already matches the requested version. I already deleted several releases on GitHub after the build process created them. I agree that it does feel slightly untidy but, in the end, it’s still better than trying to move tags which causes odd conflicts if anyone has already fetched tags since the original tag was created.
Fair enough, I didn’t include that in the change log but I’d suggest adding work items to the change log when you do them (or close to it).

In short, yes I guess the criticisms are valid but also - cut me some slack I’ve been having a hard couple of weeks!

tintoy · 2024-04-02T18:45:53Z

BTW, the reason that the PR is worded like that is because the original bug (as created) doesn’t actually describe the underlying problem. I also don’t want the PR to automatically close the linked issue (because I’d rather manually verify with the submitter and only close the issue once the release is published) so I avoid using “fixes”.

DoctorKrolic · 2024-04-02T18:49:34Z

I wish you speedy recovery, take care! About your points:

I don’t think I’ve ever seen either of us ask for a second review after addressing PR feedback. I’m not sure what the point would be but am open to discussing it.

I don't remember having so many things to adress in any previous PR. For instance, I am still concerned about 1 item that you consider fine. Also you missed some feedback (probably because github collapsed it into a hidden section). If I had a second look I would kindly remind you of leftover feedback and continue the conversation on that controversial item. I always wait for your green check before merging unless I am doing a simple dependency update without logic changes.

I agree that it does feel slightly untidy but, in the end, it’s still better than trying to move tags which causes odd conflicts if anyone has already fetched tags since the original tag was created

I am not sure about the "moving tags" part, but I will say that currently tag/release automation seems a bit overaggressive. Maybe you should tweak things a little bit (after you get better ofcourse). I'm not gonna touch things related to automation, publishing and so on, that is fully on you.

I’d suggest adding work items to the change log when you do them (or close to it)

Really good suggestion! The only problem is when the change is fully server side, in such case we need to sync server submodule more often and update upcoming changelog with server-related changes when syncing

tintoy · 2024-04-02T19:03:28Z

I’ve created a separate issue to track discussion of release process but I definitely agree with you that it needs refinement! It’s one of those things that is only ever in the way of the work you actually need to do and so doesn’t get attention until it becomes a problem. I don’t want to bog us down with unhelpful processes but I do want to make sure that whatever we do have works for everyone (not just me) 🙂

tintoy · 2024-04-02T19:04:55Z

Oh, and yes - sorry I didn’t see those other PR feedback items before because they were collapsed; I really need to get out of the habit of reading PRs on my phone (or at least to use the GitHub app not my phone’s web browser).

tintoy · 2024-04-02T19:11:43Z

Oh, and regarding the test class fields (I am assuming this is the item you’re still concerned about? Please correct me if I’ve got it wrong), I’m happy to open another issue to discuss (it sounds like a broader issue than just this test) but FWIW I’ve always found that to be a very common design pattern with xUnit tests and one that helps to keep each test as clean as possible (so you can see at a glance what it is trying to verify and it also helps to keep per-test setup consistent across tests within a suite).

tintoy · 2024-04-02T19:12:36Z

I wish you speedy recovery, take care!

Thanks 🙂

tintoy added 4 commits March 24, 2024 13:51

Add test to validate language-model components when working with an M…

1cddfe3

…SBuild property defined and then redefined in the same project file #100

Thread-safe load of sub-projects from master project

0d40200

#100

Tidy up test code

ebab95e

#100

Tidy up test code

d8aa11c

#100

tintoy requested a review from DoctorKrolic March 29, 2024 04:25

tintoy self-assigned this Mar 29, 2024

tintoy added bug area/msbuild labels Mar 29, 2024

DoctorKrolic reviewed Mar 29, 2024

View reviewed changes

src/LanguageServer.Engine/Documents/Workspace.cs Outdated Show resolved Hide resolved

tintoy added 4 commits April 1, 2024 10:23

Merge branch 'master' into bug/100-redefine-property

7cfaa2d

Refine GitVersion configuration

b8fb1ed

Configure GitVersion to ignore commit with '15.0' in its title

ac20790

Address PR feedback

b42e183

#100 #103

tintoy merged commit 918be1d into master Apr 1, 2024
1 check was pending

DoctorKrolic deleted the bug/100-redefine-property branch April 2, 2024 14:13

tintoy mentioned this pull request Apr 2, 2024

Release Process tintoy/msbuild-project-tools-vscode#146

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of concurrent loads for MSBuild sub-projects #103

Improve handling of concurrent loads for MSBuild sub-projects #103

tintoy commented Mar 29, 2024

tintoy commented Mar 29, 2024

DoctorKrolic Mar 29, 2024

tintoy Mar 29, 2024

DoctorKrolic Mar 29, 2024

tintoy Mar 29, 2024

DoctorKrolic Mar 29, 2024

tintoy Mar 29, 2024

DoctorKrolic Mar 29, 2024

tintoy Mar 29, 2024

tintoy Apr 1, 2024

DoctorKrolic Apr 2, 2024

DoctorKrolic commented Apr 2, 2024

tintoy commented Apr 2, 2024

tintoy commented Apr 2, 2024

DoctorKrolic commented Apr 2, 2024

tintoy commented Apr 2, 2024

tintoy commented Apr 2, 2024 •

edited

Loading

tintoy commented Apr 2, 2024

tintoy commented Apr 2, 2024

Improve handling of concurrent loads for MSBuild sub-projects #103

Improve handling of concurrent loads for MSBuild sub-projects #103

Conversation

tintoy commented Mar 29, 2024

tintoy commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoctorKrolic commented Apr 2, 2024

tintoy commented Apr 2, 2024

tintoy commented Apr 2, 2024

DoctorKrolic commented Apr 2, 2024

tintoy commented Apr 2, 2024

tintoy commented Apr 2, 2024 • edited Loading

tintoy commented Apr 2, 2024

tintoy commented Apr 2, 2024

tintoy commented Apr 2, 2024 •

edited

Loading