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.

Sign up for GitHub

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

Jump to bottom

Fix segfault in ExternalLoader::reloadOutdated(). #6082

Merged
merged 3 commits into from Jul 21, 2019
Merged

Fix segfault in ExternalLoader::reloadOutdated(). #6082

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b879e1
Fix segfault in ExternalLoader::reloadOutdated().
Jul 19, 2019
f18e959
Update ExternalLoader.cpp
alexey-milovidov Jul 19, 2019
e64f8e6
Add comments.
Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 32 additions & 11 deletions dbms/src/Interpreters/ExternalLoader.cpp
View file
Open in desktop
Expand Up @@ -511,12 +511,14 @@ class ExternalLoader::LoadingDispatcher : private boost::noncopyable
{
std::lock_guard lock{mutex};
for (auto & [name, info] : infos)
{
if ((info.was_loading() || load_never_loading) && filter_by_name(name))
{
cancelLoading(info);
info.forced_to_reload = true;
startLoading(name, info);
}
}
}

/// Starts reloading of all the objects.
Expand All @@ -528,20 +530,22 @@ class ExternalLoader::LoadingDispatcher : private boost::noncopyable
/// The function doesn't touch the objects which were never tried to load.
void reloadOutdated()
{
/// Iterate through all the objects and find loaded ones which should be checked if they were modified.
std::unordered_map<LoadablePtr, bool> is_modified_map;

{
std::lock_guard lock{mutex};
TimePoint now = std::chrono::system_clock::now();
for (const auto & name_and_info : infos)
{
const auto & info = name_and_info.second;
if ((now >= info.next_update_time) && !info.loading() && info.was_loading())
Copy link
Member Author

@vitlibar vitlibar Jul 19, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line caused the problem. Because if we tried to load a dictionary before but never succeeded then info.was_loading() == true, info.object == nullptr, and the function is_object_modified() was called with nullptr passed and we got segfault. Now I've changed the condition in order to call the function is_object_modified() only for loaded objects (which are not null). Anyway it doesn't make sense to check for modification a failed-to-load dictionary.

if ((now >= info.next_update_time) && !info.loading() && info.loaded())
is_modified_map.emplace(info.object, true);
}
}

/// The `mutex` should be unlocked while we're calling the function is_object_modified().
/// Find out which of the loaded objects were modified.
/// We couldn't perform these checks while we were building `is_modified_map` because
/// the `mutex` should be unlocked while we're calling the function is_object_modified().
for (auto & [object, is_modified_flag] : is_modified_map)
{
try
Expand All @@ -554,21 +558,38 @@ class ExternalLoader::LoadingDispatcher : private boost::noncopyable
}
}

/// Iterate through all the objects again and either start loading or just set `next_update_time`.
{
std::lock_guard lock{mutex};
TimePoint now = std::chrono::system_clock::now();
for (auto & [name, info] : infos)
if ((now >= info.next_update_time) && !info.loading() && info.was_loading())
{
if ((now >= info.next_update_time) && !info.loading())
{
auto it = is_modified_map.find(info.object);
if (it == is_modified_map.end())
continue; /// Object has been just added, it can be simply omitted from this update of outdated.
bool is_modified_flag = it->second;
if (info.loaded() && !is_modified_flag)
info.next_update_time = calculate_next_update_time(info.object, info.error_count);
else
if (info.loaded())
{
auto it = is_modified_map.find(info.object);
if (it == is_modified_map.end())
continue; /// Object has been just loaded (it wasn't loaded while we were building the map `is_modified_map`), so we don't have to reload it right now.

bool is_modified_flag = it->second;
if (!is_modified_flag)
{
/// Object wasn't modified so we only have to set `next_update_time`.
info.next_update_time = calculate_next_update_time(info.object, info.error_count);
continue;
}

/// Object was modified and should be reloaded.
startLoading(name, info);
}
else if (info.failed())
{
/// Object was never loaded successfully and should be reloaded.
startLoading(name, info);
}
}
}
}
}

Expand Down