Update block tip in the gui thread

[jarolrod]

This takes commit cf8a278 from #38 and rebases it over changes on master; namely that we've dropped GUIUtil::ObjectInvoke in favor of QMetaObject::invokeMethod.

This avoids non-thread-safe errors. I ran into a segfault here while running some data intensive monkey-code related to blocks, this change prevented it. I'm not going to actually use the code that caused the segfault, just to illustrate that this is useful here anyways.

[johnny9]

Does the handler need to have disconnect() called when the NodeModel cleans up possibly? I'm having a hard time seeing which object in the lambda execution would be invalid and cause the segfault.

[shaavan]

The code change looks acceptable to me. The reasoning for which is nicely explained in this Stackoverflow answer:

... The reason to use QMetaObject::invokeMethod if the recipient object might be in another thread is that attempting to call a slot directly on an object in another thread can lead to corruption or worse if it accesses or modifies non-thread-safe data.

However, I am curious about the seg fault error you mentioned in the description. Would you please add some points on what exactly you think caused the error?

[johnny9]

I agree that this change is acceptable. It's ok to do simple setters on the gui thread. My guess is that doing so forces the juicy part of the lambda to only execute while the NodeModel object is still alive but there is also the NodeImpl that adds its own wrapper to the lambda so it could be that object's lifetime as well.

[hebasto]

ACK eca5252, I have reviewed the code and it looks OK, I agree it can be merged.

#38 (review)

[promag]

but there is also the NodeImpl that adds its own wrapper to the lambda so it could be that object's lifetime as well.

@johnny9 I don't understand this, can you rephrase it?

[johnny9]

@promag

gui-qml/src/node/interfaces.cpp

Line 373 in 3984614

fn(sync_state, BlockTip{block->nHeight, block->GetBlockTime(), block->GetBlockHash()},

is the line of code I was referring to. It looks like the function takes the lambda that you pass into handleNotifyBlockTip as input (fn) and adds its own lambda around it before registering it to the handler so it can do "GuessVerificationProgress". I would think NodeImpl's lifetime would be longer than the NodeModel's though so it seemed less likely that is where the segfault is.

[promag]

The segfault comes from not calling setBlockTipHeight and setVerificationProgress on the gui thread. There is no concurrency control, and (not sure) QML direct-connects to the signals blockTipHeightChanged verificationProgressChanged.

The alternative to having some mutex is not ideal because we don't want to block the GUI and vice-versa

So the approach is to just defer the update.

Also, with QMetaObject::invokeMethod(this, fn), Qt won't call fn if this is destroyed.

[johnny9]

Does having NodeModel call m_handler_notify_block_tip.disconnect() in its deconstructor fix the segfault? If you have a reliable way to cause the error I'd like to try.

[promag]

It's already called on the m_handler_notify_block_tip destructor. But that's not the issue, setBlockTipHeight and setVerificationProgress have to be called on the GUI thread.

[johnny9]

Sorry, I guess I don't remember a reason you couldn't EMIT the signal from another thread, I thought that setting the value and emitting the signal across threads was just fine and QML engine would handle the signal within its own thread but I'm fuzzy on the details.

You are right about the handler deleting itself also. I doubled checked https://www.boost.org/doc/libs/1_57_0/doc/html/boost/signals2/scoped_connection.html.