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

Verbose logging bloated regression #2636

Closed
est31 opened this issue Apr 19, 2015 · 5 comments
Closed

Verbose logging bloated regression #2636

est31 opened this issue Apr 19, 2015 · 5 comments
Labels
Bug Issues that were confirmed to be a bug @ Network

Comments

@est31
Copy link
Contributor

est31 commented Apr 19, 2015

When you are starting minetest in singleplayer (connecting to remote servers triggers this too) and are running with the --verbose flag, you get many many messages of the kind INFO[main]: ClientEnvironment::processActiveObjectMessage(): got message for id=0, which doesn't exist.

I have found out that commit 8804c47 has caused this regression, to be more precise the change that replaced 0 with datas.size() in NetworkPacket pkt(TOCLIENT_ACTIVE_OBJECT_MESSAGES, 0, peer_id);. If you change it back to 0, the messages disappear. This isn't of course a proper fix of the root cause, which either lies in networkpacket, or some other lower layer networking code. Therefore medium priority.

@nerzhul can you have a look at this?

@est31 est31 added Bug Issues that were confirmed to be a bug @ Network Medium priority labels Apr 19, 2015
@est31
Copy link
Contributor Author

est31 commented Apr 19, 2015

Additional information: when I'm adding to handleCommand_ActiveObjectMessages the logging line errorstream << "got message with length " << datastring.length() << std::endl;, I'm seeing that in the bloating case the client gets messages of length 102, in the non-bloating case it gets messages with length 51.

@nerzhul
Copy link
Member

nerzhul commented Apr 19, 2015

Datas.size is set in packet constructor , this reserve packet size directly instead of reallocate memory in putRawString. I will look at a modification on this handler tomorrow, this copy isn't needed, we should write datas to packet directly

@est31
Copy link
Contributor Author

est31 commented Apr 19, 2015

I don't understand: Where is a copy?

@jayarndt
Copy link
Contributor

jayarndt commented May 4, 2015

I think the problem is that NetworkPacket::putRawString() has an edge case triggered by the written string being the exact same length as the free space in the packet. Then, instead of increasing the size of the packet by the amount the buffer was exceeded, it increases the packet size by the size of the written string. Thus, the packet ends up being twice the size it needs to be and padded with zeros.

The client, after processing the good first half of the packet, runs into the spurious zeros and interprets each 4 bytes as a message for object 0 of zero length. For each spurious message, the client then complains that it can't find object 0.

@est31
Copy link
Contributor Author

est31 commented May 4, 2015

2923eaf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues that were confirmed to be a bug @ Network
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants