[SENTRY-1683] MetastoreCacheInitializer has a race condition in handling results list

• Type: Bug
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 1.8.0
• Fix Version/s: 1.8.0
• Component/s: Sentry
• Assignee: Alex Kolbasov
• Reporter: Alex Kolbasov
• Created: 31/Mar/17 03:42
• Updated: 05/Apr/17 06:44
• Resolved: 05/Apr/17 06:44

Description

The MetastoreCacheInitializer has the following logic:

1) It creates a list of futures to hold results of executors (results)
2) {{CreateInitialUpdate() }}does this:

  UpdateableAuthzPaths createInitialUpdate() throws
          Exception {
    PathsUpdate tempUpdate = new PathsUpdate(-1, false);
    List<String> allDbStr = hmsHandler.get_all_databases();
    for (String dbName : allDbStr) {
      Callable<CallResult> dbTask = new DbTask(tempUpdate, dbName);
      results.add(threadPool.submit(dbTask));
    }

    while (taskCounter.get() > 0) {
      Thread.sleep(1000);
      // Wait until no more tasks remain
    }

    for (Future<CallResult> result : results) {
      CallResult callResult = result.get();

      // Fail the HMS startup if tasks are not all successful and
      // fail on partial updates flag is set in the config.
      if (!callResult.getSuccessStatus() && failOnRetry) {
        throw callResult.getFailure();
      }
    }

    authzPaths.updatePartial(Lists.newArrayList(tempUpdate),
            new ReentrantReadWriteLock());
    return authzPaths;
  }

Note that in both cases the results list is accessed without holding any locks.
But the list is also updated from tasks themselves:

  class DbTask extends BaseTask {

    @Override
    public void doTask() throws TException, SentryMalformedPathException {
     ...
      List<String> allTblStr = hmsHandler.get_all_tables(dbName);
      for (int i = 0; i < allTblStr.size(); i += maxTablesPerCall) {
        synchronized (results) {
          results.add(threadPool.submit(tableTask));
        }
      }

There is some synchronization which uses taskCounter so the second walk of the list happens when all tasks are complete, but the first walk isn't thread-safe - the list may be updated while initial tasks are created there. Note that some access are synchronized and some are not.

Issue Links

• is related to: SENTRY-1684 FullUpdateInitializer has a race condition in handling results list
• links to: Code Review