Expose InternalBufferOverflowException in FSW by ianhays · Pull Request #11785 · dotnet/corefx

ianhays · 2016-09-16T16:13:38Z

Updates the default targetgroup of filesystemwatcher to netstandard1.7 and exposes InternalBufferOverflowException out of System.IO.FileSystemWatcher.

@danmosemsft @joperezr @stephentoub

stephentoub · 2016-09-16T16:33:45Z

  <Import Project="..\dir.props" />
  <PropertyGroup>
-    <AssemblyVersion>4.0.1.0</AssemblyVersion>
+    <AssemblyVersion>4.2.0.0</AssemblyVersion>


4.2 instead of 4.1?

stephentoub · 2016-09-16T16:34:58Z

+        public InternalBufferOverflowException() { }
+        public InternalBufferOverflowException(string message) { }
+        public InternalBufferOverflowException(string message, System.Exception inner) { }
+    }


Since you're adding this, can you add it with the appropriate serialization as well? It should have a protected ctor:

protected InternalBufferOverflowException(SerializationInfo info, StreamingContext context) : base (info, context) { }

Also, the base type should be SystemException, right?

stephentoub · 2016-09-16T16:35:42Z

+            Assert.Equal(message, ide.Message);
+            Assert.Same(innerException, ide.InnerException);
+        }
+    }


Once you add the serialization stuff, you can also do a serialization test of it with https://github.com/dotnet/corefx/blob/master/src/Common/tests/System/Runtime/Serialization/Formatters/BinaryFormatterHelpers.cs#L24

What is System.Runtime.Serialization.Formatters.Binary? It's listed in that common test file but it has no package that I'm aware of.

It's the namespace that contains BinaryFormatter. BinaryFormatter is in the System.Runtime.Serialization.Formatters assembly.

joperezr · 2016-09-16T16:43:35Z

 Microsoft Visual Studio Solution File, Format Version 12.00
 # Visual Studio 14
-VisualStudioVersion = 14.0.22609.0
+VisualStudioVersion = 14.0.25123.0


Unwanted change?

joperezr · 2016-09-16T16:44:21Z

    <OutputType>Library</OutputType>
-    <NuGetTargetMoniker>.NETStandard,Version=v1.3</NuGetTargetMoniker>
+    <NuGetTargetMoniker>.NETStandard,Version=v1.7</NuGetTargetMoniker>
+    <PackageTargetFramework>netstandard1.7</PackageTargetFramework>


netstandard1.7 [](start = 4, length = 63)

You don't need this. This should now be inferred by buildtools.

joperezr · 2016-09-16T16:45:25Z

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8"?>


You don't need this depproj and project.json either. Eric made some changes such that now we harvest the latest stable package and grab old assets from there, so you can remove this.

joperezr · 2016-09-16T16:46:25Z

"netstandard1.3": {

No need for this anymore and no need for the net46 either. What you do need to add is a net463 since we have to x-compile for it

Refers to: src/System.IO.FileSystem.Watcher/src/project.json:3 in 6c17624. [](commit_id = 6c17624, deletion_comment = False)

joperezr · 2016-09-16T16:47:10Z

  <PropertyGroup>
    <ProjectGuid>{77E702D9-C6D8-4CE4-9941-D3056C3CCBED}</ProjectGuid>
+    <AssemblyVersion Condition="'$(TargetGroup)'=='netstandard1.3' Or '$(TargetGroup)'=='net46' Or '$(TargetGroup)'=='netcore50'">4.0.0.0</AssemblyVersion>
+    <ContractProject Condition="'$(AssemblyVersion)'=='4.0.0.0'">../ref/4.0.0/System.IO.FileSystem.Watcher.depproj</ContractProject>


You don't need this anymore.

joperezr · 2016-09-16T16:47:21Z

    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'net46'">true</IsPartialFacadeAssembly>
-    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.3</NuGetTargetMoniker>
+    <NuGetTargetMoniker Condition="'$(TargetGroup)' == 'netstandard1.3'">.NETStandard,Version=v1.3</NuGetTargetMoniker>


.NETStandard,Version=v1.3 [](start = 4, length = 115)

Remove this line as well.

joperezr · 2016-09-16T16:47:41Z

+    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.7</NuGetTargetMoniker>
  </PropertyGroup>
-  <PropertyGroup Condition="'$(TargetsWindows)' == 'true' AND '$(TargetGroup)' == ''">
+  <PropertyGroup Condition="'$(TargetsWindows)' == 'true' AND ('$(TargetGroup)' == '' Or '$(TargetGroup)' == 'netstandard1.3')">


Or '$(TargetGroup)' == 'netstandard1.3' [](start = 86, length = 39)

We won't compile for netstandard1.3 anymore so this can go away too

Oh good, that saves me a ton of hassle. From looking at the project files for IO and FileSystem it appeared that we still needed to be able to build for the older netstandard1.3. There's a bunch of junk in this PR that can be removed in that case.

joperezr · 2016-09-16T16:52:06Z

@ianhays you are missing a few things in order for this to work for netstandard1.7

You need to change the src/System.IO.FileSystem.Watcher.builds file to add a x-compilation for net463 and remove the one for net46.
You need to make sure that the src/.csproj builds correctly for net463, so basically you want to check all of the places where we have a condition like '$(TargetGroup)' == 'net46' in the project and change that to 463.
You need to update the pkg/.pkgproj file to say that you now support netcoreapp1.1, like:

  <ItemGroup>
    <ProjectReference Include="..\ref\System.IO.FileSystem.Watcher.csproj">
      <SupportedFramework>net463;netcoreapp1.1;$(AllXamarinFrameworks)</SupportedFramework>
    </ProjectReference>
    <ProjectReference Include="..\src\System.IO.FileSystem.Watcher.builds" />
  </ItemGroup>

Updates the default targetgroup of filesystemwatcher to netstandard1.7 and exposes InternalBufferOverflowException out of System.IO.FileSystemWatcher.

ianhays · 2016-09-27T16:05:12Z

@joperezr @stephentoub Thanks for all the feedback! I responded to it all and pushed some updates, though it seems the new github interface doesn't handle rebases too well.

joperezr · 2016-09-28T21:59:23Z

-    "net46": {
+    "net463": {
      "dependencies": {
        "Microsoft.TargetingPack.NETFramework.v4.6": "1.0.1"


v4.6 [](start = 46, length = 4)

we should probably update this to be v4.6.2

joperezr · 2016-09-28T22:00:33Z

        public System.IO.WaitForChangedResult WaitForChanged(System.IO.WatcherChangeTypes changeType) { return default(System.IO.WaitForChangedResult); }
        public System.IO.WaitForChangedResult WaitForChanged(System.IO.WatcherChangeTypes changeType, int timeout) { return default(System.IO.WaitForChangedResult); }
    }
+    [Serializable]


Serializable [](start = 5, length = 12)

Just curious, should this type implement ISerializable? I ask because usually types that have this attribute also have that interface and it's implementation.

Just curious, should this type implement ISerializable?

not according to the docs

Exception itself implements ISerializable, and it exposes a protected GetObjectData method that a derived exception can override if it has additional state that needs to be serialized... InternalBufferOverflowException doesn't, so it doesn't.

In general, the majority of [Serializable] types don't actually implement ISerializable. When a type is marked as [Serializable], BinaryFormatter does a default serialization, where it serializes all fields (unless they're marked as [NonSerialized]). If you want to customize that behavior, there are a variety of ways, one of which is implementing ISerializable (which provides the GetObjectData method for getting the state to be serialized) and providing a deserialization ctor (one that takes SerializationInfo and StreamingContext)... when you implement ISerializable, BinaryFormatter then defers to the GetObjectData+ctor to serialize and deserialize the instance.

I see, thanks for the explanation @stephentoub 😄

joperezr · 2016-09-28T22:03:07Z

@@ -1,25 +1,26 @@
 {


I'm not sure I understand why there is a separate project.json for netstandard1.7 if this is also includded on the one under src/ . Is there a reason why we can't clean this up?

joperezr · 2016-09-28T22:12:59Z

+        "opensuse.13.2-x64",
+        "opensuse.42.1-x64"
+      ]
+    }


I believe you shouldn't need this anymore. All you need now is to add a "coreFx.Test.netcoreapp1.1": {}, and that should be good enough since we recently updated the nuget framework mappings which understand the connection between netstandard1.7 and netcoreapp1.1

joperezr · 2016-09-28T22:13:24Z

+    "netstandard1.3": {},
+    "netstandard1.7": {
+      "dependencies": {
+        "System.Runtime.Serialization.Formatters": "4.3.0-beta-24522-03"


"System.Runtime.Serialization.Formatters": "4.3.0-beta-24522-03" [](start = 8, length = 64)

Any reason why we can't just add this where the rest of the dependencies are?

Formatters doesn't work when the TargetGroup is netstandard1.3.

joperezr

Added a few comments but overall this LGTM.

stephentoub · 2016-09-30T14:31:25Z

Thanks, Ian.

* Expose InternalBufferOverflowException in FSW Updates the default targetgroup of filesystemwatcher to netstandard1.7 and exposes InternalBufferOverflowException out of System.IO.FileSystemWatcher. Commit migrated from dotnet/corefx@148e7d9

ianhays added the dev/api-file io label Sep 16, 2016

ianhays added this to the 1.1.0 milestone Sep 16, 2016

ianhays self-assigned this Sep 16, 2016

dnfclas added the cla-already-signed label Sep 16, 2016

stephentoub reviewed Sep 16, 2016

View reviewed changes

joperezr reviewed Sep 16, 2016

View reviewed changes

danmoseley added area-System.IO netstandard2.0 and removed area-System.IO dev/api-file io labels Sep 16, 2016

karelz removed this from the 1.1.0 milestone Sep 23, 2016

Ian Hays added 3 commits September 26, 2016 16:11

Expose InternalBufferOverflowException in FSW

8b9fa39

Updates the default targetgroup of filesystemwatcher to netstandard1.7 and exposes InternalBufferOverflowException out of System.IO.FileSystemWatcher.

Update project.json versions

d482915

Update package assembly version

0aeaf40

joperezr reviewed Sep 28, 2016

View reviewed changes

joperezr approved these changes Sep 28, 2016

View reviewed changes

PR feedback.

ab7ca63

stephentoub merged commit 148e7d9 into dotnet:master Sep 30, 2016

karelz modified the milestone: 1.2.0 Dec 3, 2016

ianhays deleted the api_fsw_internalbufferoverflowexception branch April 25, 2017 18:05

Conversation

ianhays commented Sep 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Sep 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Sep 16, 2016

Uh oh!

ianhays commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Sep 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants