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

Mark FaultException as Serializable #4205

Merged

Conversation

imcarolwang
Copy link
Contributor

For issue #3984 .

@HongGit ,@mconnew , @StephenBonikowsky
I am not sure if mark the FaultException class as Serializable is enough to make the scenario function work, let me know your concerns.

Copy link
Member

@mconnew mconnew 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 also be added to the reference assembly. Can you add a simple serialization test to the unit tests for S.SM.Primitives to ensure the serialized payload is the same between NetFx and Core.

@imcarolwang
Copy link
Contributor Author

This should also be added to the reference assembly. Can you add a simple serialization test to the unit tests for S.SM.Primitives to ensure the serialized payload is the same between NetFx and Core.

Now I've added the attribute to the ref project file System.ServiceModel.Primitives.cs.

And I tried to serialize a FaultException instance with XmlSerializer, it doesn't work in .Net Framework nor .Net Core due to error about the base class "Exception" implements IDictionary.

I can use BinaryFormater to do serialization for FaultException type but serialized payload comparison with NetFramework is different. Any suggestion? Btw, deserialization from the serialized MemoryStream won't work as there seems has other dependencies not implemented, shall we support that as well?

@mconnew
Copy link
Member

mconnew commented Mar 20, 2020

XmlSerializer can't serialize IDictionary, but DataContractSerializer can. I couldn't find an instance of MemoryStream on FaultException or Exception. Where are you seeing this?

@imcarolwang
Copy link
Contributor Author

XmlSerializer can't serialize IDictionary, but DataContractSerializer can. I couldn't find an instance of MemoryStream on FaultException or Exception. Where are you seeing this?

I forgot to mention I tried DataContractSerializer, it threw exception with message:

System.Runtime.Serialization.InvalidDataContractException : Type 'System.ServiceModel.FaultException+FaultCodeData' cannot be serialized. Consider marking it with the DataContractAttribute attribute, and marking all of its members you want serialized with the DataMemberAttribute attribute. Alternatively, you can ensure that the type is public and has a parameterless constructor - all public members of the type will then be serialized, and no attributes will be required.

Regarding the MemoryStream thing, I am sorry it's not clear said, I meant after serialize FaultException instance to a memory stream, I was trying to deserialize it back, but failed to do so.

@mconnew
Copy link
Member

mconnew commented Mar 27, 2020

The FaultCodeData class also needs the Serializable attribute applied. If you compare the NetFx source with the .NET Core source, you can see that the attribute has been applied in NetFx.

@imcarolwang
Copy link
Contributor Author

imcarolwang commented Mar 30, 2020

Thanks, @mconnew .

I've updated the product and test codes. Serializaing FaultException instance with DataContractSerializer would get same payload as .NetFramework baseline after making following changes:

  1. mark class FaultCodeData and FaultReasonData with [Serializable] attribute
  2. add method public override void GetObjectData(SerializationInfo info, StreamingContext context) and its dependencies to FaultException.cs (.net source ref)
  3. rename the private variable names in class FaultCodeData and FaultReasonData.
  4. update the original .NetFramework baseline string section
    <HResult i:type=""x:int"" xmlns="""">-2146233087</HResult> to
    <HResult i:type=""x:int"" xmlns="""">-2146233088</HResult>

Does the changes in step1,2,3 make sense to you? Is result in step4 expected?

@mconnew
Copy link
Member

mconnew commented Apr 30, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mconnew
Copy link
Member

mconnew commented Apr 30, 2020

Your step 4 is interesting. On Framework, CommunicationException derives from SystemException which sets the default HResult to COR_E_SYSTEM (-2146233087). SystemException didn't exist in Core until netstandard2.0 so we had to derive from Exception instead, which defaults HResult to COR_E_EXCEPTION (-2146233088). This is where you are seeing the change. I'll open an issue around whether we should switch to SystemException as our base type. For now we'll include that change for the tests as it's caused by the base class difference.

@mconnew
Copy link
Member

mconnew commented Apr 30, 2020

Tests are failing because the Windows machines are running with the Spanish culture "es-ES" and the test expects "en-US".

@mconnew
Copy link
Member

mconnew commented Apr 30, 2020

@imcarolwang, can you modify the FaultExceptionTest.Serializable_Default test to capture the current culture before instantiating the FaultException, set the current culture to en-US, instantiate the FaultException object and then restore the current culture before continuing with the test? Some test machines purposefully have a different culture to catch product issues which don't show up with en-US so we need to be tolerant of running on a different culture.

@mconnew
Copy link
Member

mconnew commented May 1, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mconnew
Copy link
Member

mconnew commented May 1, 2020

@imcarolwang, don't worry about my last comment. I've fixed the build to use the English Windows image.

@mconnew mconnew merged commit 91b1b05 into dotnet:master May 1, 2020
@imcarolwang imcarolwang deleted the issue3984_FaultExceptionSerialization branch July 8, 2021 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants