fix disposing issues and incorrect work in netcore2.0 #38
Conversation
There was a problem hiding this comment.
Hi @marfenij, thanks for the PR! I've added some comments. Could you please describe the actual bug fix(es) that you made, so I can mention it in the release notes?
Some advice for future pull requests; it is better to have multiple small pull requests than to have a single large pull request with multiple (unrelated) changes. This makes it more easy to review, modify and, if necessary, easier to roll back. The removal of RhinoMocks and SharpTestsEx is good work, and would be perfect for a separate (small) pull request. For now it's fine though. :)
| @@ -341,8 +341,7 @@ public bool IsDBNull(int i) | |||
|
|
|||
| public void Close() | |||
| { | |||
| stream.Close(); | |||
| IsClosed = true; | |||
| this.Dispose(); | |||
There was a problem hiding this comment.
I don't think the Close() method should dispose the reader. I've checked the behavior of SqlDataReader and OdbcDataReader and the reader is not disposed when it is closed.
There was a problem hiding this comment.
You are right - correct way call Close() in Dispose method
| @@ -88,7 +88,7 @@ public void ShouldThrowExceptionWhenBufferOffsetIsLessThanZero() | |||
| dataReader.Read(); | |||
|
|
|||
| // Assert | |||
| Assert.Throws<ArgumentOutOfRangeException>("bufferOffset", () => dataReader.GetBytes(0, 0, new byte[1], -1, 1)); | |||
There was a problem hiding this comment.
Checking the parameter name ensures that the expected exception at the expected location has been thrown.
There was a problem hiding this comment.
I understand, but old xunit, which used for net40 not support that argument
| @@ -100,7 +100,7 @@ public void ShouldThrowExceptionWhenBufferOffsetIsGreaterThanBufferSize() | |||
| dataReader.Read(); | |||
|
|
|||
| // Assert | |||
| Assert.Throws<ArgumentOutOfRangeException>("bufferOffset", () => dataReader.GetBytes(0, 0, new byte[1], 10, 1)); | |||
There was a problem hiding this comment.
Checking the parameter name ensures that the expected exception at the expected location has been thrown.
There was a problem hiding this comment.
As I mentioned above, this is old xUnit issue
| @@ -112,7 +112,7 @@ public void ShouldThrowExceptionWhenBufferOffsetIsEqualToBufferSize() | |||
| dataReader.Read(); | |||
|
|
|||
| // Assert | |||
| Assert.Throws<ArgumentOutOfRangeException>("bufferOffset", () => dataReader.GetBytes(0, 0, new byte[1], 1, 1)); | |||
There was a problem hiding this comment.
Checking the parameter name ensures that the expected exception at the expected location has been thrown.
There was a problem hiding this comment.
As I mentioned above, this is old xUnit issue
| @@ -88,7 +88,7 @@ public void ShouldThrowExceptionWhenBufferOffsetIsLessThanZero() | |||
| dataReader.Read(); | |||
|
|
|||
| // Assert | |||
| Assert.Throws<ArgumentOutOfRangeException>("bufferOffset", () => dataReader.GetChars(0, 0, new char[1], -1, 1)); | |||
There was a problem hiding this comment.
Checking the parameter name ensures that the expected exception at the expected location has been thrown.
There was a problem hiding this comment.
As I mentioned above, this is old xUnit issue
.gitignore
Outdated
| [Rr]elease*/ | ||
| Ankh.NoLoad | ||
| .nuget/nuget.exe | ||
| .vs/ |
There was a problem hiding this comment.
.vs/ is already mentioned on line 40.
.gitignore
Outdated
| @@ -98,6 +98,13 @@ ipch/ | |||
| *.cachefile | |||
| *.VC.db | |||
| *.VC.VC.opendb | |||
| obj/ | |||
There was a problem hiding this comment.
obj/ is already mentioned on line 36.
.gitignore
Outdated
| @@ -98,6 +98,13 @@ ipch/ | |||
| *.cachefile | |||
| *.VC.db | |||
| *.VC.VC.opendb | |||
| obj/ | |||
| [Bb]in | |||
There was a problem hiding this comment.
Already mentioned on line 28 - 37.
.gitignore
Outdated
| [Bb]in | ||
| [Dd]ebug*/ | ||
| [Rr]elease*/ | ||
| Ankh.NoLoad |
There was a problem hiding this comment.
Not sure what this is (Ankh.NoLoad).
.gitignore
Outdated
| @@ -98,6 +98,13 @@ ipch/ | |||
| *.cachefile | |||
There was a problem hiding this comment.
The changes made to .gitignore mostly seem to be double entries, better drop the changes to .gitignore.
There was a problem hiding this comment.
sorry for that, i revert .gitignore to original state https://www.gitignore.io/api/visualstudio,visualstudiocode
|
Thank you for review. I try made small changes for PR in future )) (also I know that feature I missed include, and that feature helps me debug that bug with buffers sizes) So, on of problem it's incorrect assuming that protobuf-net always return 1024 byte in buffer. this two thing cause runtime exception that internal (circular) buffer overflow |
|
@dotarj also some trouble with MSBuild.SonarQube.Runner on AppVeyor build |
|
Thanks for updating the PR, it's merged. There is a (sonarqube) problem with the build with pull requests from forks (secure variables are not available for security). I'll fix this later. |
|
@dotarj great!! |
So, I extend test projects to several target frameworks, remove unsupported stuff like RhinoMocks, SharpTestsEx; append test to check close/dispose workflow, and check behavior when buffer sizes changes (at this moment I got exceptions on real project)