Buffer too small problem in NpgsqlCopySerializer #188

Merged
merged 3 commits into from Apr 28, 2014

Projects

None yet

2 participants

@asgeirh
asgeirh commented Mar 18, 2014

This patch fixes the issue in NpgsqlCopySerializer

@asgeirh asgeirh changed the title from Fixing issue #187 Buffer too small problem in NpgsqlCopySerializer to Buffer too small problem in NpgsqlCopySerializer Mar 18, 2014
@franciscojunior
Member

Hi, @asgeirh !
I'm checking your pull request. Thanks for your contribution!

Would you mind to copy your explanation of this patch from the issue you closed? Or add a link to it here. This would help us understand the rationale of your patch.
Also, can you please check the indentation of your patch? It seems you add much more spaces :)

Would be very nice if you could add a simple test of this problem. This would help us prevent this problem from happening again.

Thanks in advance.

asgeirh Fixed spaces
Added Test case
04c4866
@asgeirh
asgeirh commented Mar 19, 2014

I added Test case and fixed the Space issue also here is the.
I hope the commit is now in the pull request I am a github novice ;)

Orginal error I was having:
The buffer error is as follows:
System.ArgumentException : The output byte buffer is too small to contain the encoded data, encoding 'Unicode (UTF-8)' fallback 'System.Text.EncoderReplacementFallback'.
Parameter name: bytes

at System.Text.Encoding.ThrowBytesOverflow(EncoderNLS encoder, Boolean nothingEncoded)
at System.Text.UTF8Encoding.GetBytes(Char* chars, Int32 charCount, Byte* bytes, Int32 byteCount, EncoderNLS baseEncoder)
at System.Text.UTF8Encoding.GetBytes(String s, Int32 charIndex, Int32 charCount, Byte[] bytes, Int32 byteIndex)
at Npgsql.NpgsqlCopySerializer.AddString(String fieldValue) in d:\opensource\Npgsql\Npgsql\Npgsql\NpgsqlCopySerializer.cs:line 552
at NpgsqlTests.CommandTests.BufferNpgsqlCopySerializer() in d:\opensource\Npgsql\tests\CommandTests.cs:line 3446

@franciscojunior
Member

Hi, @asgeirh !

Can you talk more about your fix?

Thanks in advance.

@asgeirh
asgeirh commented Apr 5, 2014

@franciscojunior This fix is when you add to the CopySerializer data that is over DEFAULT_BUFFER_SIZE (8k) and there is some data already in buffer. This the MakeRoomForBytes function took the length needed and made a new buffer with that size and then copied the data that was in the buffer to the new buffer. So the new buffer was too small. so my fix is simple take the bytes needed + bytes already in buffer.

I assume this is the reason people are always using .Flush after each row is finished in examples.

@franciscojunior franciscojunior added this to the 2.2 milestone Apr 8, 2014
@franciscojunior
Member

Hi, @asgeirh !
Thanks for clarifying it. :)

I know it is exactly the same, but what do you think about change the line which allocate more bytes to something like:

BufferSize += len - SpaceInBuffer;

I think this line would be more readable than using the internal field _sendBufferAt.

I think we could even change it to:

int increaseBufferSize = len - SpaceInBuffer;
BufferSize += increaseBufferSize;

Both ways, those changes would be to improve the readability of the code.

What do you think?

@asgeirh
asgeirh commented Apr 28, 2014

@franciscojunior I have made the changes you suggested to make the code more readable.

Only thing I don't like but can be fixed later, is in the beginning of the method MakeRoomForBytes it checks if the _sendBuffer is null if so it initializes the buffer to current BufferSize not the needed size. So its potential that it will allocate the buffer two times. I am not sure how often this would happen but to me its ugly. But a matter for another ticket.

@franciscojunior
Member

Hi, @asgeirh !

Thanks for your changes.

I also agree with you regarding the beginning of MakeRoomForBytes.

We can work on it in another issue as you suggested. :)

I'm merging your changes.

Thanks for your feedback and patch! :)

@franciscojunior franciscojunior merged commit 468df53 into npgsql:master Apr 28, 2014

1 check passed

default Finished TeamCity Build Npgsql :: All : Running
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment