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

Resend blocks when modified while sending to client #3713

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@Jeija
Copy link
Contributor

commented Feb 15, 2016

From what I understand, this can happen when the server sends node updates to the client:

  • More than 4 nodes change at the same time (server.cpp:888)
  • This causes the server to only send nodes that are 5 nodes or less away from the player (server.cpp:911), for any other nodes the corresponding position in m_blocks_sent in RemoteClient will be removed to mark those blocks for resending.
  • Server::SendBlocks calls RemoteClient::GetNextBlocks which returns among others the block that was just removed from m_blocks_sent
  • Here is the problem: While the block is transferred to the client, the block on the server is modified again. The corresponding position in m_blocks_sent is marked for resending again.
  • Before the server can send the new blocks, the client answers with TOSERVER_GOTBLOCKS, which causes the block in m_blocks_sent to be unmarked (=marked as sent = inserted into m_blocks_sent):
void RemoteClient::GotBlock(v3s16 p)
{
    /** some other stuff here **/
    m_blocks_sent.insert(p);
}
  • Result: The server thinks the client's copy of the block is up to date when in fact, the block has been modified in the meantime.

You might think this issue is completely hypothetical due to the short time between sending the block and receiving the answer from the client, but it causes major issues when using mods that modify many nodes at the same time, such as mesecons:

These are not mesecons bugs, but caused by minetest itself:

You can easily reproduce this issue by building a small mesecons circuit, most annoyingly using pistons. You will get results like these (screenshots were taken in singleplayer):
screenshot_20160215_164205
screenshot_20160215_170452
And it is not just a matter of waiting a few seconds for the server to resend the block, you have to manually cause a block update in order to see what is really going on.

Solution

My patch makes the server remember which blocks were modified, so it can ignore the TOSERVER_GOTBLOCKS answers from the client for those blocks that changed since last sending an update.

@Megaf

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

Sounds great! I will give it a try and report back later.
Thanks!

@est31

View changes

src/clientiface.cpp Outdated
}

void RemoteClient::SentBlock(v3s16 p)
{
if(m_blocks_modified.find(p) != m_blocks_modified.end())

This comment has been minimized.

Copy link
@est31

est31 Feb 16, 2016

Contributor

Space between if and (

@est31

View changes

src/clientiface.cpp Outdated
{
m_excess_gotblocks++;
if(m_blocks_sending.find(p) != m_blocks_sending.end())

This comment has been minimized.

Copy link
@est31

est31 Feb 16, 2016

Contributor

Space between if and (

@est31

View changes

src/clientiface.cpp Outdated
m_excess_gotblocks++;
if(m_blocks_sending.find(p) != m_blocks_sending.end())
m_blocks_sending.erase(p);
else

This comment has been minimized.

Copy link
@est31

est31 Feb 16, 2016

Contributor

put the brace on the same line as the else, separed by one space

@est31

View changes

src/clientiface.cpp Outdated
if(m_blocks_sending.find(p) != m_blocks_sending.end())
m_blocks_sending.erase(p);
else
if (m_blocks_modified.find(p) == m_blocks_modified.end())

This comment has been minimized.

Copy link
@est31

est31 Feb 16, 2016

Contributor

brace on same line as ).

@est31

View changes

src/clientiface.h Outdated
client reports it has received them to account for blocks
that are being modified while on the line.

Key is position, value is dummy

This comment has been minimized.

Copy link
@est31

est31 Feb 16, 2016

Contributor

No need to replicate the value is dummy for m_blocks_sent. It is a mistake left over from commit 6a1670d (the commit replaced map with set but didnt update the comment).

@est31

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

See comments above. Otherwise looks good.

@est31 est31 added the Bug label Feb 16, 2016

@Jeija Jeija force-pushed the Jeija:fix_blockupdate branch Feb 17, 2016

@Jeija

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2016

Thank you @est31, I have updated my first commit according to your suggestions.
While doing that though, I found many more the style issues in clientiface.cpp (actually there are more if( than if (). So I added a second commit to this PR that would fix all those issues.

So, feel free to merge only the first commit, it contains the style fixes to my code you suggested, the second just contains some more for the rest of the code.

@Jeija Jeija force-pushed the Jeija:fix_blockupdate branch 2 times, most recently Feb 17, 2016

@Jeija Jeija force-pushed the Jeija:fix_blockupdate branch to 1b54f0f Feb 17, 2016

@est31

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2016

Its a good idea to have it merged as two commits.

@kilbith

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2016

@est31 0.4.14 milestone ?

@nerzhul nerzhul added this to the 0.4.14 milestone Feb 24, 2016

@est31 est31 added the One approval label Mar 8, 2016

@orwell96

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2016

We definitely need this.
I lately recognized a bug with placing locked chests or other types of nodes that are setting up their inventory in after_place_node callback which appears since my PR regarding m_nothing_to_send_pause_timer.

  1. client sends: placing chest
  2. server queues mapblock for resending immediately
  3. server calls after_place node
  4. client answers with gotblocks and server thinks the client already knows about the inventory being created.
    Result: The client does not know about the inventory and shows an empty formspec.
    Ironically, i initially set m_nothing_to_send_pause_timer to 0.1 instead of 0.

@est31 est31 added >= Two approvals and removed One approval labels Mar 12, 2016

@est31

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2016

Okay, nore has +1ed the PR, see IRC log: http://irc.minetest.ru/minetest-dev/2016-03-12#i_4555202

Merged as 089f9bb
only the first commit. @Jeija I couldn't apply the second commit patch, can you rebase it and add it in a separate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.