Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix regression in third party ParameterInfo serialization. #12038

Merged
merged 1 commit into from Jun 14, 2017
Merged

Fix regression in third party ParameterInfo serialization. #12038

merged 1 commit into from Jun 14, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jun 1, 2017

Fix https://github.com/dotnet/corefx/issues/20574

The decision to make the runtime's implementation of
Reflection objects non-serializable went too far
and changed the behaviors of methods inherited by
all Reflection implementors. In particular, by
changing ParameterInfo.GetRealObject() to throw PNSE,
all third-party implementations of ParameterInfo become
undeserializable, not just the runtime's implementation.
The intended change was to make the runtime's implementation
non-serializable which is already accomplished by removing
[Serializable] from RuntimeParameterInfo.

The methods on the abstract base classes for Reflection
are "apis" to all Reflection implementors and their long-standing
behaviors should not change based on decisions made
by the runtime's particular implementation.

This commit rolls back those changes that were introduced
as part of #12020.

Fix https://github.com/dotnet/corefx/issues/20574

The decision to make the runtime's implementation of
Reflection objects non-serializable went too far
and changed the behaviors of methods inherited by
all Reflection implementors. In particular, by
changing ParameterInfo.GetRealObject() to throw PNSE,
all third-party implementations of ParameterInfo become
undeserializable, not just the runtime's implementation.
The intended change was to make the runtime's implementation
non-serializable which is already accomplished by removing
[Serializable] from RuntimeParameterInfo.

The methods on the abstract base classes for Reflection
are "apis" to all Reflection implementors and their long-standing
behaviors should not change based on decisions made
by the runtime's particular implementation.

This commit rolls back those changes that were introduced
as part of #12020.
@ghost ghost requested review from danmoseley and morganbr June 1, 2017 20:30
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Makes sense.

@ghost
Copy link
Author

ghost commented Jun 1, 2017

@dotnet-bot test OSX10.12 x64 Checked Build and Test

Low disk space...

@ghost
Copy link
Author

ghost commented Jun 1, 2017

@dotnet-bot test OSX10.12 x64 Checked Build and Test

@danmoseley danmoseley added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 7, 2017
@danmoseley
Copy link
Member

waiting on shiproom

@stephentoub
Copy link
Member

waiting on Shiproom

@danmosemsft, still?

@danmoseley
Copy link
Member

Thanks for the reminder, I pinged.

@danmoseley
Copy link
Member

@dotnet-bot test this please (since it's been a while, just to be sure0

shiproom approved to merge whne green

@danmoseley
Copy link
Member

@mmitche ERROR: [WS-CLEANUP] Cannot delete workspace: remote file operation failed: /Users/dotnet-bot/j/workspace/dotnet_coreclr/release_2.0.0/checked_osx10.12_tst_prtest at hudson.remoting.Channel@626598f8:dci-mac-build-048: hudson.remoting.ChannelClosedException: channel is already closed
@dotnet-bot test OSX10.12 x64 Checked Build and Test please

@danmoseley
Copy link
Member

@dotnet-bot test tizen armel cross debug build please (sev fault in b12263)

@danmoseley
Copy link
Member

@dotnet-bot test windows_nt x64 debug build and test (JIT_Generics._Coverage_chaos65204782cs_chaos65204782cs_._Coverage_chaos65204782cs_chaos65204782cs_cmd)

@danmoseley danmoseley removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 14, 2017
@danmoseley danmoseley merged commit 5f696b0 into dotnet:release/2.0.0 Jun 14, 2017
@danmoseley
Copy link
Member

@atsushikan after some babysitting - this is now in.

@ghost ghost deleted the serrestore branch June 14, 2017 13:45
@mmitche
Copy link
Member

mmitche commented Jun 14, 2017

@danmosemsft @dotnet/eng-first These are network issues. Whenever you see that, the easiest way to 'fix' it is to set the machine to offline.

@danmoseley
Copy link
Member

@mmitche I didn't know I had the power. That's in the Jenkins UI? OK I will look next time. Once offline I just ping you and restart the job? Taht's what I did anyway.

@mmitche
Copy link
Member

mmitche commented Jun 14, 2017

@danmosemsft Yep. The issue is that the network having issues and immediately failing jobs, but Jenkins will prioritize using the same machine for the next job (and it looks idle anyway). That leads to the string of jobs failing.

The issue is 'fixed' in a new version of jenkins, so we'll get it soon. Basically if the network drops (as is the case here), Jenkins needs to kill all of the open communication channels as soon as it detects this.

@danmoseley
Copy link
Member

@mmitche I see. when do we expect to pick up a new Jenkins?

@mmitche
Copy link
Member

mmitche commented Jun 14, 2017

@danmosemsft After 2.0. Probably beginning of august.

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants