Improve handling of concurrent loads for MSBuild sub-projects #103
Conversation
…SBuild property defined and then redefined in the same project file #100
|
@DoctorKrolic , yeah, sorry, got a bit carried away and added more doc comments 😂 |
| @@ -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. | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah fair call, I’ll update it to match for now but we can sort it out properly later (e.g. #91).
| @@ -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>(); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I’m ok with either var or new() as long as the type name appears somewhere on the line. Will update!
| MasterProject = new MasterProjectDocument(this, documentUri, Log); | ||
| return MasterProject; | ||
| } | ||
| return MasterProject = new MasterProjectDocument(this, documentUri, Log); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Fair enough; I don’t feel particularly strongly about their usage but if they’re already causing problems I’m happy to remove it.
| /// <summary> | ||
| /// The project collection for any projects loaded by the current test. | ||
| /// </summary> | ||
| ProjectCollection _projectCollection; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)…
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/TestProjects/RedefineProperty.SameFile.csproj
Show resolved
Hide resolved
|
@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 |
|
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:
In short, yes I guess the criticisms are valid but also - cut me some slack I’ve been having a hard couple of weeks! |
|
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”. |
|
I wish you speedy recovery, take care! About your points:
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 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.
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 |
|
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) 🙂 |
|
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). |
|
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). |
Thanks 🙂 |
Relates to #100, but it turns out the actual issue is related to concurrent loads of sub-projects, not overridden properties.