You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org> on 2018/03/14 07:54:01 UTC
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9626
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
IMPALA-6488: removes use-after-free bug in lib_cache
Several recent runs have resulted in a boost mutex
invalid argument exception. The mutex in question is
the one that guards individual lib_cache entries (LibCacheEntry::lock).
The exception is thrown due to the entry being deleted
by another thread prior to the current thread acquiring
the entry lock. This scenario happens when:
1) thread t1 looks up an existing entry e
a. the lookup is guarded by a cache lock
b. the cache lock is released (L356)
c. e's lock is acquired on the next line and propagated
up the call stack (look for the swaps)
d. while e is locked, its use-count is incremented.
2) thread t2 deletes entry e
a. the cache lock is acquired and e is looked up
b. e's lock is acquired
c. if e's usecount is 0 and was marked for removal, e is deleted.
If t2 runs following (1b), then t1 will acquire a lock on a deleted
entry (1c), causing the mutex exception.
There are two parts to the fix in this change:
1) remove 1b: keep the cache lock while acquiring e's lock. The cache lock is
then released and all other operations proceed as before. Note that current
lock ordering is still maintained (coarse to fine), but now, the cache lock can be held
while other threads block access to the entry lock. When files have been copied,
these operations are short. While files are loading, accesses to the same
entry can serialize access to the entire cache.
2) add cache entry after its loaded. Currently, on a cache miss, a new entry is
created, locked, added to the cache, and finally the cache is unlocked. The intent
is to allow other threads to concurrently load files for other entries. However,
now that the cache lock is held while the entry lock is held, the loading thread
will block other thread's from acquiring the same entry, which will block access
to the cache. The work-around is to release the cache lock when loading a new
entry and add the cache entry only when its loaded. The workaround avoids
expensive work while holding the cache lock, but may do wasted work if multiple
threads miss on the same entry and load their entries in parallel.
Testing:
- ran core tests
- added an end-to-end test that concurrently uses and drops/creates from the
same lib-cache entry. with current settings, the use-after-free error is
reliably reproduced.
Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
---
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M tests/query_test/test_udfs.py
3 files changed, 191 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/9626/1
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
IMPALA-6488: removes use-after-free bug in lib_cache
Several recent runs have resulted in a boost mutex
invalid argument exception. The mutex in question is
the one that guards individual lib_cache entries (LibCacheEntry::lock).
The exception is thrown due to the entry being deleted
by another thread prior to the current thread acquiring
the entry lock. This scenario happens when:
1) thread t1 looks up an existing entry e
a. the lookup is guarded by a cache lock
b. the cache lock is released (L356)
c. e's lock is acquired on the next line and propagated
up the call stack (look for the swaps)
d. while e is locked, its use-count is incremented.
2) thread t2 deletes entry e
a. the cache lock is acquired and e is looked up
b. e's lock is acquired
c. if e's usecount is 0 and was marked for removal, e is deleted.
If t2 runs following (1b), then t1 will acquire a lock on a deleted
entry (1c), causing the mutex exception.
There are two parts to the fix in this change:
1) remove 1b: keep the cache lock while acquiring e's lock. The cache lock is
then released and all other operations proceed as before. Note that current
lock ordering is still maintained (coarse to fine), but now, the cache lock can be held
while other threads block access to the entry lock. When files have been copied,
these operations are short. While files are loading, accesses to the same
entry can serialize access to the entire cache.
2) add cache entry after its loaded. Currently, on a cache miss, a new entry is
created, locked, added to the cache, and finally the cache is unlocked. The intent
is to allow other threads to concurrently load files for other entries. However,
now that the cache lock is held while the entry lock is held, the loading thread
will block other thread's from acquiring the same entry, which will block access
to the cache. The work-around is to release the cache lock when loading a new
entry and add the cache entry only when its loaded. The workaround avoids
expensive work while holding the cache lock, but may do wasted work if multiple
threads miss on the same entry and load their entries in parallel.
Testing:
- ran core tests
- added an end-to-end test that concurrently uses and drops/creates from the
same lib-cache entry. with current settings, the use-after-free error is
reliably reproduced.
Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
---
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M tests/query_test/test_udfs.py
3 files changed, 184 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/9626/2
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@157
PS3, Line 157: /// refreshed. The cache lock is required to be held prior to calling this method.
also mention the entry locking behavior (could copy sentence on line 143-144)
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@159
PS3, Line 159: LibMap::iterator& iter
is that modified (i.e. returned)? assuming no, it should be 'const ref'. (if it were an out param, then we generally would use pointer rather than ref).
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@160
PS3, Line 160: LibCacheEntry** entry);
it'd be nice to put all the "out params" last (i.e. put entry_lock after iter).
http://gerrit.cloudera.org:8080/#/c/9626/2/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9626/2/be/src/runtime/lib-cache.cc@249
PS2, Line 249: lib_cache_.erase(entry_iter);
Nit: let’s not add the extraneous parenthesis
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc@320
PS3, Line 320: cache lock is
: // *not* held
Let's add the "why" -- something about loading the entry being expensive.
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc@394
PS3, Line 394: RETURN_IF_ERROR((*entry)->copy_file_status);
the error case here seems wrong. shouldn't we set *entry to nullptr and not hold the lock (similar to when loading_status had an error, and the other error paths in this function)? or does the caller somehow distinguish these two error cases? (in which case the header comment for RefreshCacheEntry needs updating).
Looking deeper... oh crazy, the caller does distinguish -- see line 293 in GetCacheEntry();
if (*entry == NULL) return status;
// Set loading_status on the entry so that if another thread calls
// GetCacheEntry() for this lib before this thread is able to acquire lock_ in
// RemoveEntry(), it is able to return the same error.
(*entry)->loading_status = status;
This is preexisting weirdness, I guess, so maybe not worth mucking with but at the least noting in the header comments.
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 23:42:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has removed Michael Brown from this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Removed reviewer Michael Brown.
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
David, does the test look okay to you now?
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.h@156
PS4, Line 156: loading_status
This comment still seems inaccurate since this routine doesn't use loading_status to propagate the error to the caller. the status is propagated to the caller by the return value, and it's the caller's caller that ultimately sticks it in loading_status to propagate it to other threads (and ultimately might be noticed by this routine in a subsequent invokation, but then in that case, a null entry is returned). (And strangely we end up calling RemoveEntry() in both cases, but just in different places).
How about we just delete this sentence since I don't think it's possible to succinctly describe the interface that this code follows (a pre-existing problem)?
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.cc@325
PS4, Line 325: // other
newline glitch
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 03:47:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 4:
extra changes in last patches are to covert NULL -> nullptr uniformly in lib-cache.cc
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 01:38:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 19:50:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:54:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello David Knupp, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9626
to look at the new patch set (#5).
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
IMPALA-6488: removes use-after-free bug in lib_cache
Several recent runs have resulted in a boost mutex
invalid argument exception. The mutex in question is
the one that guards individual lib_cache entries
(LibCacheEntry::lock).
The exception is thrown due to the entry being deleted
by another thread prior to the current thread acquiring
the entry lock. This scenario happens when:
1) thread t1 looks up an existing entry e
a. the lookup is guarded by a cache lock
b. the cache lock is released (L356)
c. e's lock is acquired on the next line and propagated
up the call stack (look for the swaps)
d. while e is locked, its use-count is incremented.
2) thread t2 deletes entry e
a. the cache lock is acquired and e is looked up
b. e's lock is acquired
c. if e's usecount is 0 and was marked for removal, e is deleted.
If t2 runs following (1b), then t1 will acquire a lock on a deleted
entry (1c), causing the mutex exception.
There are two parts to the fix in this change:
(1) don't crash and (2) don't let concurrency regress.
1) remove 1b: keep the cache lock while acquiring e's lock.
The cache lock is then released and all other operations proceed as before.
Note that current lock ordering is still maintained (coarse to fine),
but now, the cache lock can be held while other threads block access
to the entry lock. When files have been copied, these operations
are short. While files are loading, accesses to the same entry
can serialize access to the entire cache.
2) add cache entry after its loaded.
Currently, on a cache miss, a new entry is created, locked,
added to the cache, and finally the cache is unlocked. The intent
is to allow other threads to concurrently load files for other entries.
However, now that the cache lock is held while the entry lock is held,
the loading thread will block other thread's from acquiring the same entry,
which will block access to the cache. The work-around is to release
the cache lock when loading a new entry and add the cache entry only when
its loaded. The workaround avoids expensive work while holding the
cache lock, but may do wasted work if multiple threads miss on the
same entry and load their entries in parallel.
Testing:
- ran core tests + hdfs
- added an end-to-end test that concurrently uses and drops/creates from
the same lib-cache entry. with current settings, the use-after-free
error is reliably reproduced.
- manual testing to examine concurrency of the following cases:
- concurrent function creation from multiple lib files
(stresses coordinator)
- concurrent function use from multiple lib files
(stresses backend)
Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
---
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M tests/query_test/test_udfs.py
3 files changed, 219 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/9626/5
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@157
PS3, Line 157: /// refreshed. The cache lock is required to be held prior to calling this method.
> also mention the entry locking behavior (could copy sentence on line 143-14
Done
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@159
PS3, Line 159: LibMap::iterator& iter
> is that modified (i.e. returned)? assuming no, it should be 'const ref'. (i
Done
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@160
PS3, Line 160: LibCacheEntry** entry);
> it'd be nice to put all the "out params" last (i.e. put entry_lock after it
Done
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc@320
PS3, Line 320: cache lock is
: // *not* held
> Let's add the "why" -- something about loading the entry being expensive.
Done
http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc@394
PS3, Line 394: RETURN_IF_ERROR((*entry)->copy_file_status);
> the error case here seems wrong. shouldn't we set *entry to nullptr and not
right. the status is saved in the entry for the error case. added some clarifying comments since, yes, this is bit convoluted.
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 01:36:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@328
PS2, Line 328: setup = create_fn_to_use.format(unique_database, '/test-warehouse/libTestUdfs.so')
Nit: this could probably go before the try block.
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@331
PS2, Line 331: print e
You can print/log any extra info you like here, but why not then raise the actual exception, rather then forcing an entirely new one using an assert? E.g.:
try:
setup_client.execute(setup_fn)
except Exception as e:
print "Unable execute initial create function: {0}".format(setup_fn) # or something...
raise
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@341
PS2, Line 341: except Exception as e: errors.append(e)
PEP-8 discourages inlining like this. Break after the colon (throughout).
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@351
PS2, Line 351: except Exception as e: errors.append(e)
Break errors.append(e) onto a new line.
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@358
PS2, Line 358: for i in range(0, num_uses):
Nit: a bit pedantic, but xrange is favored over range in python 2.x. (In fact, range has been simply replaced by xrange in 3.x.).
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 21:12:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello David Knupp, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9626
to look at the new patch set (#3).
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
IMPALA-6488: removes use-after-free bug in lib_cache
Several recent runs have resulted in a boost mutex
invalid argument exception. The mutex in question is
the one that guards individual lib_cache entries
(LibCacheEntry::lock).
The exception is thrown due to the entry being deleted
by another thread prior to the current thread acquiring
the entry lock. This scenario happens when:
1) thread t1 looks up an existing entry e
a. the lookup is guarded by a cache lock
b. the cache lock is released (L356)
c. e's lock is acquired on the next line and propagated
up the call stack (look for the swaps)
d. while e is locked, its use-count is incremented.
2) thread t2 deletes entry e
a. the cache lock is acquired and e is looked up
b. e's lock is acquired
c. if e's usecount is 0 and was marked for removal, e is deleted.
If t2 runs following (1b), then t1 will acquire a lock on a deleted
entry (1c), causing the mutex exception.
There are two parts to the fix in this change:
(1) don't crash and (2) don't let concurrency regress.
1) remove 1b: keep the cache lock while acquiring e's lock.
The cache lock is then released and all other operations proceed as before.
Note that current lock ordering is still maintained (coarse to fine),
but now, the cache lock can be held while other threads block access
to the entry lock. When files have been copied, these operations
are short. While files are loading, accesses to the same entry
can serialize access to the entire cache.
2) add cache entry after its loaded.
Currently, on a cache miss, a new entry is created, locked,
added to the cache, and finally the cache is unlocked. The intent
is to allow other threads to concurrently load files for other entries.
However, now that the cache lock is held while the entry lock is held,
the loading thread will block other thread's from acquiring the same entry,
which will block access to the cache. The work-around is to release
the cache lock when loading a new entry and add the cache entry only when
its loaded. The workaround avoids expensive work while holding the
cache lock, but may do wasted work if multiple threads miss on the
same entry and load their entries in parallel.
Testing:
- ran core tests + hdfs
- added an end-to-end test that concurrently uses and drops/creates from
the same lib-cache entry. with current settings, the use-after-free
error is reliably reproduced.
- manual testing to examine concurrency of the following cases:
- concurrent function creation from multiple lib files
(stresses coordinator)
- concurrent function use from multiple lib files
(stresses backend)
Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
---
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M tests/query_test/test_udfs.py
3 files changed, 186 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/9626/3
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 2:
(5 comments)
thanks for the review!
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@328
PS2, Line 328: setup = create_fn_to_use.format(unique_database, '/test-warehouse/libTestUdfs.so')
> Nit: this could probably go before the try block.
Done
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@331
PS2, Line 331: print e
> You can print/log any extra info you like here, but why not then raise the
Done
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@341
PS2, Line 341: except Exception as e: errors.append(e)
> PEP-8 discourages inlining like this. Break after the colon (throughout).
Done
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@351
PS2, Line 351: except Exception as e: errors.append(e)
> Break errors.append(e) onto a new line.
Done
http://gerrit.cloudera.org:8080/#/c/9626/2/tests/query_test/test_udfs.py@358
PS2, Line 358: for i in range(0, num_uses):
> Nit: a bit pedantic, but xrange is favored over range in python 2.x. (In fa
Done
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 21:47:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
IMPALA-6488: removes use-after-free bug in lib_cache
Several recent runs have resulted in a boost mutex
invalid argument exception. The mutex in question is
the one that guards individual lib_cache entries
(LibCacheEntry::lock).
The exception is thrown due to the entry being deleted
by another thread prior to the current thread acquiring
the entry lock. This scenario happens when:
1) thread t1 looks up an existing entry e
a. the lookup is guarded by a cache lock
b. the cache lock is released (L356)
c. e's lock is acquired on the next line and propagated
up the call stack (look for the swaps)
d. while e is locked, its use-count is incremented.
2) thread t2 deletes entry e
a. the cache lock is acquired and e is looked up
b. e's lock is acquired
c. if e's usecount is 0 and was marked for removal, e is deleted.
If t2 runs following (1b), then t1 will acquire a lock on a deleted
entry (1c), causing the mutex exception.
There are two parts to the fix in this change:
(1) don't crash and (2) don't let concurrency regress.
1) remove 1b: keep the cache lock while acquiring e's lock.
The cache lock is then released and all other operations proceed as before.
Note that current lock ordering is still maintained (coarse to fine),
but now, the cache lock can be held while other threads block access
to the entry lock. When files have been copied, these operations
are short. While files are loading, accesses to the same entry
can serialize access to the entire cache.
2) add cache entry after its loaded.
Currently, on a cache miss, a new entry is created, locked,
added to the cache, and finally the cache is unlocked. The intent
is to allow other threads to concurrently load files for other entries.
However, now that the cache lock is held while the entry lock is held,
the loading thread will block other thread's from acquiring the same entry,
which will block access to the cache. The work-around is to release
the cache lock when loading a new entry and add the cache entry only when
its loaded. The workaround avoids expensive work while holding the
cache lock, but may do wasted work if multiple threads miss on the
same entry and load their entries in parallel.
Testing:
- ran core tests + hdfs
- added an end-to-end test that concurrently uses and drops/creates from
the same lib-cache entry. with current settings, the use-after-free
error is reliably reproduced.
- manual testing to examine concurrency of the following cases:
- concurrent function creation from multiple lib files
(stresses coordinator)
- concurrent function use from multiple lib files
(stresses backend)
Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Reviewed-on: http://gerrit.cloudera.org:8080/9626
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M tests/query_test/test_udfs.py
3 files changed, 219 insertions(+), 98 deletions(-)
Approvals:
Dan Hecht: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 7
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 5: Code-Review+1
Test looks fine to me. Someone else should +2.
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:52:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.h@156
PS4, Line 156: loading_status
> This comment still seems inaccurate since this routine doesn't use loading_
yes, its too specific to how its used. trimmed down and added a todo to clean this up...
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.cc@325
PS4, Line 325: // other
> newline glitch
Done
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 04:06:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello David Knupp, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9626
to look at the new patch set (#4).
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
IMPALA-6488: removes use-after-free bug in lib_cache
Several recent runs have resulted in a boost mutex
invalid argument exception. The mutex in question is
the one that guards individual lib_cache entries
(LibCacheEntry::lock).
The exception is thrown due to the entry being deleted
by another thread prior to the current thread acquiring
the entry lock. This scenario happens when:
1) thread t1 looks up an existing entry e
a. the lookup is guarded by a cache lock
b. the cache lock is released (L356)
c. e's lock is acquired on the next line and propagated
up the call stack (look for the swaps)
d. while e is locked, its use-count is incremented.
2) thread t2 deletes entry e
a. the cache lock is acquired and e is looked up
b. e's lock is acquired
c. if e's usecount is 0 and was marked for removal, e is deleted.
If t2 runs following (1b), then t1 will acquire a lock on a deleted
entry (1c), causing the mutex exception.
There are two parts to the fix in this change:
(1) don't crash and (2) don't let concurrency regress.
1) remove 1b: keep the cache lock while acquiring e's lock.
The cache lock is then released and all other operations proceed as before.
Note that current lock ordering is still maintained (coarse to fine),
but now, the cache lock can be held while other threads block access
to the entry lock. When files have been copied, these operations
are short. While files are loading, accesses to the same entry
can serialize access to the entire cache.
2) add cache entry after its loaded.
Currently, on a cache miss, a new entry is created, locked,
added to the cache, and finally the cache is unlocked. The intent
is to allow other threads to concurrently load files for other entries.
However, now that the cache lock is held while the entry lock is held,
the loading thread will block other thread's from acquiring the same entry,
which will block access to the cache. The work-around is to release
the cache lock when loading a new entry and add the cache entry only when
its loaded. The workaround avoids expensive work while holding the
cache lock, but may do wasted work if multiple threads miss on the
same entry and load their entries in parallel.
Testing:
- ran core tests + hdfs
- added an end-to-end test that concurrently uses and drops/creates from
the same lib-cache entry. with current settings, the use-after-free
error is reliably reproduced.
- manual testing to examine concurrency of the following cases:
- concurrent function creation from multiple lib files
(stresses coordinator)
- concurrent function use from multiple lib files
(stresses backend)
Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
---
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M tests/query_test/test_udfs.py
3 files changed, 221 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/9626/4
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9626/2/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9626/2/be/src/runtime/lib-cache.cc@249
PS2, Line 249: lib_cache_.erase(entry_iter);
> Nit: let’s not add the extraneous parenthesis
Ignore that, you already fixed it.
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 23:42:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:53:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2103/
--
To view, visit http://gerrit.cloudera.org:8080/9626
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:54:50 +0000
Gerrit-HasComments: No