Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/937#discussion_r76537936
--- Diff: iocore/net/Socks.cc ---
@@ -136,7 +136,7 @@ void
SocksEntry::free()
{
MUTEX_TRY_LOCK(lock, action_.mutex, this_ethread());
- if (lock.is_locked()) {
+ if (!lock.is_locked()) {
// Socks continuation share the user's lock
// so acquiring a lock shouldn't fail
ink_assert(0);
--- End diff --
This reads a little strange, and has contradicting semantics in debug build
(where it'll abort) vs release builds (where the call to SocksEntry::free()
silently fails).
Would it make more sense to change this to an ink_release_assert()? E.g.
ink_release_assert(lock.is_locked);
? If not, then the invariant is not strong enough, and likely should be
changed to not be an assert at all, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
