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

Few critical bugs are fixed #97

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

georgemavchun
Copy link

Few bug fixes included.
In each commit there is test that fails without fix commit and doesn't fail with it.

writeBuffer = ByteBuffer.allocate(writeBufferSize);
this.objectBufferSize = objectBufferSize;
lengthLength = serialization.getLengthLength();
writeBuffer = ByteBuffer.allocate(Math.max(writeBufferSize, lengthLength + objectBufferSize));
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the write buffer size that was passed in?

Copy link
Author

Choose a reason for hiding this comment

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

Because bufferSize >= lengthLength + objectSize must be true in all cases.
If it is not so, user has entered incorrect values. We can react on that two ways:
first - throw an exception, second - use max(bufferSize, lengthLength + objectSize).
We have chosen second one, but we can change to exception.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I prefer an exception to using a buffer size other than what was specified.

@NathanSweet
Copy link
Member

Combining multiple fixes in the same PR makes review difficult. Please explain the fixes.

@georgemavchun
Copy link
Author

Should we delete that PR and resubmit few new? Or comments we have written is enough?

@georgemavchun
Copy link
Author

So what should we do now?

@CreamyCookie
Copy link

@NathanSweet Any news on this?

@NathanSweet
Copy link
Member

Splitting it into multiple PRs would help a lot.

ai-dsxt added 2 commits June 19, 2019 12:21
Dsx 2: Check classes registered in the client and the server are the same
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants