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.

Sign up for GitHub

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

Jump to bottom

Fix files kept in use in XslTransformation #6946

Merged
merged 1 commit into from
Oct 15, 2021
Merged

Fix files kept in use in XslTransformation #6946

merged 1 commit into from
Oct 15, 2021

Conversation

lanfeust69
Copy link
Contributor

@lanfeust69 lanfeust69 commented Oct 13, 2021 •
edited by benvillalobos
Loading

Fixes #6962

Work item (Internal use): https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1417029

Summary

The XslTransformation now holds a lock on its input xml file.

Customer Impact

Customers that expect to modify/use the input files to the XslTransformation task are unable to due to a lock on the file.

Regression?

Yes, in VS 17.0 P5+

Testing

Added regression test & customer verified the fix.

Risk

Low: makes use of a parameter that was previously passed null. This parameter now tells the xmlreader to drop its lock on the file as soon as it's done.

Original Post

Following #6863, the created XmlReader is no longer responsible for its
underlying stream. This can cause the build process to hold on to the
processed file, preventing its removal. This can especially be a problem
when the transformation is in fact aimed at the input file itself, where
we want to create the transformed file, then move it to the original.

@dnfadmin
Copy link

dnfadmin commented Oct 13, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@dnfadmin
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ lanfeust69 sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

benvillalobos
benvillalobos approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

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

This change looks fine overall, with some nits. I tested deleting the file in the unit test without your change and the task indeed holds onto the file.

src/Tasks.UnitTests/XslTransformation_Tests.cs
@@ -386,7 +393,7 @@ public void OutputTest()
/// Setting correct "Parameter" parameters for Xsl.
/// </summary>
[Fact]
public void XsltParamatersCorrect()
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the change is here?

Copy link
Member

Choose a reason for hiding this comment

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

There's a typo in "parameters"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry about that, I'm on the verge of OCD for these kind of things, and when I spotted that while skimming through the tests, I couldn't resist...

src/Tasks.UnitTests/XslTransformation_Tests.cs Outdated
if (xmlInputs[xmi].Key == XslTransformation.XmlInput.XmlModes.XmlFile)
{
string xmlInputPath = ((TaskItem[])xmlInputs[xmi].Value)[0].ItemSpec;
File.Delete(xmlInputPath);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
File.Delete(xmlInputPath);
File.Delete(xmlInputPath);
File.Exists(xmlInputPath).ShouldBeFalse();

Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would be a separate test so we explicitly know xsltransform shouldn't be holding on to a file. Can we move this to its own test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will push shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, this showed that the xsl was also kept locked (I'm pretty sure this was the case before, sure it should rarely be an issue in real life, but still...)

benvillalobos
benvillalobos approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

src/Tasks.UnitTests/XslTransformation_Tests.cs Outdated Show resolved Hide resolved
src/Tasks.UnitTests/XslTransformation_Tests.cs Outdated Show resolved Hide resolved
@benvillalobos
Copy link
Member

This fixes a potential regression that made it to 17.0, but I'm not sure how realistic it is someone could run into this. @lanfeust69 how did you discover this bug?

cc @rainersigwald

@lanfeust69
Copy link
Contributor Author

This fixes a potential regression that made it to 17.0, but I'm not sure how realistic it is someone could run into this. @lanfeust69 how did you discover this bug?

cc @rainersigwald

Because it broke our build, of course 😄 !

Our specific use-case is post-processing App.config files after they have been completed with binding redirects, to handle multi-targeting net48 and netcore, with "well-known" sections such as system.data.

Forgind
Forgind approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

This should probably point to main instead of vs17.1, but I think it's otherwise ready to go.

src/Tasks.UnitTests/XslTransformation_Tests.cs Outdated Show resolved Hide resolved
@lanfeust69
Fix files kept in use in XslTransformation
8cd6f36 
Following dotnet#6863, the created XmlReader is no longer responsible for its
underlying stream. This can cause the build process to hold on to the
processed file, preventing its removal. This can especially be a problem
when the transformation is in fact aimed at the input file itself, where
we want to create the transformed file, then move it to the original.
@benvillalobos benvillalobos changed the base branch from vs17.1 to vs17.0 October 14, 2021 20:57
@benvillalobos
Copy link
Member

benvillalobos commented Oct 14, 2021 •
edited
Loading

This should probably point to main instead of vs17.1

The PR that caused this regression made it to 17.0 so I pointed to vs17.0. We should get this approved, merge it, and let it propagate up.

cc @dotnet/kitten

@benvillalobos benvillalobos mentioned this pull request Oct 14, 2021
Open
@benvillalobos benvillalobos merged commit d66a440 into dotnet:vs17.0 Oct 15, 2021
@benvillalobos
Copy link
Member

Thanks for the contribution @lanfeust69!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benvillalobos benvillalobos benvillalobos approved these changes

@Forgind Forgind Forgind approved these changes

@tmeschter tmeschter tmeschter approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

Assignees
No one assigned
Labels
Servicing-consider
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lanfeust69 @dnfadmin @benvillalobos @rainersigwald @tmeschter @Forgind