Skip to content

Conversation

danmoseley
Copy link
Member

Fix #65235 ... hopefully.

The XML tests are very old, and thus messy. Making significant changes is a non goal and I'm not certain what caused this failure.

In separated commit order

  • fixed cases where a MemoryStream would get disposed at an arbitrary point by a wrapping StreamWriter, causing ODE. Instead, deterministically dispose the wrapper, without closing the inner stream. Remove redundant flushes. I don't believe this is the cause of the test failure.
  • cleaned up the elaborate static cache of MemoryStreams, since it was locking around accessing the cache, but generally handing out the same streams to all comers. Potentially could have caused the test failure seen, if two threads are meddling with the offset of the stream, and copying is happening during: the copy could end up with nulls. The complexity of these tests is such that I can't reason whether this could happen. Instead, always clone to a new stream before handing it out.
  • fixed the test exception to not crash in some failure cases, especially as it happens on a threadpool thread
  • fixed a test to test what it claims to test per its comment
  • fixed a case where a test expected to receive the same stream from the cache that it inserted - broken by the 2nd change

@ghost ghost added the area-System.Xml label Feb 14, 2022
@ghost ghost assigned danmoseley Feb 14, 2022
@ghost
Copy link

ghost commented Feb 14, 2022

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #65235 ... hopefully.

The XML tests are very old, and thus messy. Making significant changes is a non goal and I'm not certain what caused this failure.

In separated commit order

  • fixed cases where a MemoryStream would get disposed at an arbitrary point by a wrapping StreamWriter, causing ODE. Instead, deterministically dispose the wrapper, without closing the inner stream. Remove redundant flushes. I don't believe this is the cause of the test failure.
  • cleaned up the elaborate static cache of MemoryStreams, since it was locking around accessing the cache, but generally handing out the same streams to all comers. Potentially could have caused the test failure seen, if two threads are meddling with the offset of the stream, and copying is happening during: the copy could end up with nulls. The complexity of these tests is such that I can't reason whether this could happen. Instead, always clone to a new stream before handing it out.
  • fixed the test exception to not crash in some failure cases, especially as it happens on a threadpool thread
  • fixed a test to test what it claims to test per its comment
  • fixed a case where a test expected to receive the same stream from the cache that it inserted - broken by the 2nd change
Author: danmoseley
Assignees: -
Labels:

area-System.Xml

Milestone: -

{
MemoryStream ms = new MemoryStream();
TextWriter tw = new StreamWriter(ms);
using var tw = new StreamWriter(ms, encoding:null, bufferSize:-1, leaveOpen:true);
Copy link
Member

Choose a reason for hiding this comment

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

nit: should we put space after :?

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually don't know what our prevailing style is. I will have to check. @sharwell is there any StyleCop or editorconfig linting of this available?

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

Positive changes, looks like removing some potential culprits, just a single nit around style (foo:bar vs foo: bar), thanks for the PR!

@danmoseley
Copy link
Member Author

@dotnet/dncenghot can you please help with these?

2022-02-14T03:12:09.1609457Z D:\a\_work\1\s\.packages\microsoft.dotnet.helix.sdk\7.0.0-beta.22110.7\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(55,5): error : Helix encountered job-level error(s) for this job (Exception thrown while: sending work items to Service Bus).  Please contact dnceng with this information. [D:\a\_work\1\s\src\libraries\sendtohelixhelp.proj]
2022-02-14T03:12:09.1894357Z ##[error].packages\microsoft.dotnet.helix.sdk\7.0.0-beta.22110.7\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(55,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Helix encountered job-level error(s) for this job (Exception thrown while: sending work items to Service Bus).  Please contact dnceng with this information.

@MattGal
Copy link
Member

MattGal commented Feb 14, 2022

@dotnet/ d n ce n ghot can you please help with these?

2022-02-14T03:12:09.1609457Z D:\a\_work\1\s\.packages\microsoft.dotnet.helix.sdk\7.0.0-beta.22110.7\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(55,5): error : Helix encountered job-level error(s) for this job (Exception thrown while: sending work items to Service Bus).  Please contact dnceng with this information. [D:\a\_work\1\s\src\libraries\sendtohelixhelp.proj]
2022-02-14T03:12:09.1894357Z ##[error].packages\microsoft.dotnet.helix.sdk\7.0.0-beta.22110.7\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(55,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Helix encountered job-level error(s) for this job (Exception thrown while: sending work items to Service Bus).  Please contact dnceng with this information.

@lpatalas I think was looking earlier but I'll take a look too to make sure it's not ongoing. I'd expect this to be a burst of trouble with the Service Bus endpoint we were talking to and not a persistent issue since work has continued to be enqueued this whole time.

@MattGal
Copy link
Member

MattGal commented Feb 15, 2022

@danmoseley this was an outage from Service bus clustered between 2/13/2022 6:12:08.241 PM -> 2/13/2022, 8:47:12.666 PM PST. It self resolved and was external to us, so hopefully just a blip.

@danmoseley danmoseley merged commit c1ee3cd into dotnet:main Feb 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Xml.Tests.CNameTableTestModule.RunTests failure on Windows Server 2022
5 participants