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

fix for when we only have one message type #102

Merged
merged 3 commits into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@jarrettchisholm

jarrettchisholm commented Oct 10, 2018

I'm not sure why, but it looks like the total number of message types was being subtracted by one. if I remove that subtraction, it seems to fix the issue.

@gafferongames

This comment has been minimized.

Show comment
Hide comment
@gafferongames

gafferongames Oct 10, 2018

Member

While this fixes it, it's not perfectly efficient.

If you have 16 message types, you are serializing a type value in the range [0,15], hence the -1.

The problem occurs when the number of message types is 1, then it subtracts and get zero.

The code as written is attempting to go, oh, there is only one message type, so don't serialize the message type as an integer, since serializing an int in range [0,0] is undefined.

Most likely, there is a bug in the code such that there is uninitialized data on read in one of the serialize functions, such that the message type is not defaulted to 0. This is my guess anyway.

Could you try fixing this in such a way that it remains efficient? eg. keeps the -1, but special cases the numMessageTypes = 1 case.

Thanks!

Member

gafferongames commented Oct 10, 2018

While this fixes it, it's not perfectly efficient.

If you have 16 message types, you are serializing a type value in the range [0,15], hence the -1.

The problem occurs when the number of message types is 1, then it subtracts and get zero.

The code as written is attempting to go, oh, there is only one message type, so don't serialize the message type as an integer, since serializing an int in range [0,0] is undefined.

Most likely, there is a bug in the code such that there is uninitialized data on read in one of the serialize functions, such that the message type is not defaulted to 0. This is my guess anyway.

Could you try fixing this in such a way that it remains efficient? eg. keeps the -1, but special cases the numMessageTypes = 1 case.

Thanks!

@jarrettchisholm

This comment has been minimized.

Show comment
Hide comment
@jarrettchisholm

jarrettchisholm Oct 10, 2018

Okay cool.

I gave it a shot, let me know what you think.

jarrettchisholm commented Oct 10, 2018

Okay cool.

I gave it a shot, let me know what you think.

@gafferongames gafferongames merged commit e65878b into networkprotocol:master Oct 10, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gafferongames

This comment has been minimized.

Show comment
Hide comment
@gafferongames

gafferongames Oct 10, 2018

Member

As long as it works when there is only one message type, and passes all unit tests (which it just did), I'm down. Thanks!

Member

gafferongames commented Oct 10, 2018

As long as it works when there is only one message type, and passes all unit tests (which it just did), I'm down. Thanks!

@FredGithub

This comment has been minimized.

Show comment
Hide comment
@FredGithub

FredGithub Oct 21, 2018

Contributor

I had no issue with a test project with only one message type, without the fix in question, on both Windows and Linux.

Plus, I'm not sure if the pull request would fix anything. The only changes are on a bunch of asserts and the removal of __builtin_clz, but as I said, I had no issue with it, with only one message type.

Can you please give more details about your issue or provide a test project?

And more importantly, can we revert the pull request? The changes make the behavior wrong IMO, since you're not supposed to make a 0 bits write, and even then, the code is now inconsistent (other asserts later down the line still check that bits is > 0)

Contributor

FredGithub commented Oct 21, 2018

I had no issue with a test project with only one message type, without the fix in question, on both Windows and Linux.

Plus, I'm not sure if the pull request would fix anything. The only changes are on a bunch of asserts and the removal of __builtin_clz, but as I said, I had no issue with it, with only one message type.

Can you please give more details about your issue or provide a test project?

And more importantly, can we revert the pull request? The changes make the behavior wrong IMO, since you're not supposed to make a 0 bits write, and even then, the code is now inconsistent (other asserts later down the line still check that bits is > 0)

@gafferongames

This comment has been minimized.

Show comment
Hide comment
@gafferongames

gafferongames Oct 21, 2018

Member

Yes, the issue only manifested on certain compilers and systems. It's not surprising that you cannot repro it. I was never able to either.

Member

gafferongames commented Oct 21, 2018

Yes, the issue only manifested on certain compilers and systems. It's not surprising that you cannot repro it. I was never able to either.

@gafferongames

This comment has been minimized.

Show comment
Hide comment
@gafferongames

gafferongames Oct 21, 2018

Member

@FredGithub can you please make a pull request that fixes or reverts this change?

Member

gafferongames commented Oct 21, 2018

@FredGithub can you please make a pull request that fixes or reverts this change?

@FredGithub

This comment has been minimized.

Show comment
Hide comment
@FredGithub

FredGithub Oct 21, 2018

Contributor

Okay. @jarrettchisholm can you explain the intention behind the changes you made? Also, do you happen to have another OS you could test this issue on?

What is the issue exactly? Lost messages? Crash?

Contributor

FredGithub commented Oct 21, 2018

Okay. @jarrettchisholm can you explain the intention behind the changes you made? Also, do you happen to have another OS you could test this issue on?

What is the issue exactly? Lost messages? Crash?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment