Page MenuHome

Fix Node UI Storage Threading Issues
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Mar 1 2021, 11:11 PM.

Details

Summary

Since the same node tree can be used in modifiers on different objects,
there can be multiple threads writing to the maps in the node tree UI
storage at the same time. The additions for attribute name hints and
error messages made it so this would often cause a crash or at least
an ASAN report. This patch adds locks to prevent multiple threads
from using the maps concurrently.

In a brief test I actually didn't observe a crash without the global
bNodeTree UI storage mutex, but I think it's necessary for the change
to be correct, and I did notice some unfreed memory without it anyway.
Ideally it would be in a node tree runtime struct though.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Mar 1 2021, 11:11 PM
Hans Goudey (HooglyBoogly) created this revision.
source/blender/blenkernel/intern/node_ui_storage.cc
44

This works, but a double checked locking should work here and reduces threading overhead.

if (ntree.ui_storage != nullptr) {
  return;
}
std::lock_guard<std::mutex> lock(global_ui_storage_mutex);
if (ntree.ui_storage != nullptr) {
  return;
}
ntree.ui_storage = new NodeTreeUIStorage();
87

This lock does not seem necessary.

Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 2 2021, 12:43 PM
This revision now requires changes to proceed.Mar 2 2021, 12:43 PM
  • Add double lock optimization, remove unecessary lock
This revision is now accepted and ready to land.Mar 2 2021, 6:06 PM
This revision was automatically updated to reflect the committed changes.