Remove dangerous lock compare method and make potential usage obvious
ClosedAll Users
Actions

Authored by timcharper on Jun 21 2017, 6:35 AM.

Details

Reviewers

jenkins
jeschkies
kensipe
jdef

Commits

rMARATHON4baf75a1bead: Remove dangerous lock compare method and make potential usage obvious

JIRA Issues

JIRA MARATHON-7541 Dangerous dead-lock prone method in Marathon

Test Plan

run integration tests

Diff Detail

Repository

rMARATHON marathon

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Changes from before your most recent comment are hidden. Show Older Changes

Cool!

src/main/scala/mesosphere/marathon/util/Lock.scala
44	I'd be actually radical and crash Marathon in these cases. I'm fine with logging it for now. We don't have proper guidelines for logging and error handling yet anyways.

src/main/scala/mesosphere/marathon/util/Lock.scala
44	That's really not a bad idea, @jeschkies . :)

Closed by commit rMARATHON4baf75a1bead: Remove dangerous lock compare method and make potential usage obvious (authored by timcharper). · Explain WhyJun 23 2017, 9:42 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents

			Path	Packages
M			src/main/scala/mesosphere/marathon/util/Lock.scala (29 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	3583	faa0c1e		Jun 21 2017, 6:35 AM	★	★
Diff 2	3621	99d1539	rMARATHON4baf75a1beadf04768b4fa9e301347494a43c807	Jun 23 2017, 9:42 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
8486e1473f31	18683577bdfa	faa0c1eea282	Tim Harper	Remove dangerous lock compare method and make potential usage obvious (Show More…)	Jun 21 2017, 6:23 AM

Diff 3621

View Options

src/main/scala/mesosphere/marathon/util/Lock.scala

1	1		package mesosphere.marathon
2	2		package util
3	3
	4		import com.typesafe.scalalogging.StrictLogging
4	5		import java.util.concurrent.locks.{ ReentrantLock, ReentrantReadWriteLock }
5	6
6	7		class RichLock(val lock: ReentrantLock) extends AnyVal {
7	8		def apply[T](f: => T): T = {
8	9		lock.lock()
9	10		try {
10	11		f
11	12		} finally {
12	13		lock.unlock()
13	14		}
14	15		}
15	16		}
16	17
17	18		object RichLock {
18	19		def apply(fair: Boolean = true): RichLock = new RichLock(new ReentrantLock(fair))
19	20		def apply(lock: ReentrantLock): RichLock = new RichLock(lock)
20	21		}
21	22
22			class Lock[T](private val value: T, fair: Boolean = true) {
	23		class Lock[T](private val value: T, fair: Boolean = true) extends StrictLogging {
23	24		private val lock = RichLock(fair)
24	25
25	26		def apply[R](f: T => R): R = lock {
26	27		f(value)
27	28		}
28	29
29			override def equals(o: Any): Boolean = o match {
30			case r: Lock[T] => lock {
31			r.lock {
32			value.equals(r.value)
33			}
34			}
35			case r: T @unchecked => lock {
36			value.equals(r)
37			}
38			case _ => false
	30		/** Previously, this method was too smart and had a serious flaw. Given lock a and lock b, the following could
	31		* deadlock:
	32		*
	33		* Future { a == b }
	34		* Future { b == a }
	35		*
	36		* Throwing an exception seems safer than changing the behavior, as this way existing attempts to rely on this
	37		* behavior are much more obvious.
	38		*
	39		* After some time, we can remove the exception and use the default Java equals method (compare identity).
	40		*/
	41		override def equals(o: Any): Boolean = {
	42		val ex = new RuntimeException(
	43		"Comparing two locked values was not safe and is no longer allowed.")
	44		logger.error("attempted usage of equals; remove!", ex)
		jdefUnsubmitted Not Done I commented on this pattern elsewhere: why log with `error`, including the exception, and then also throw the same exact exception? almost certainly will result in redundant stack traces in the logs. Suggest using a lower log level (like warn or info). I don't think we have a best practice around this though
		kensipeUnsubmitted Not Done this is one of those overarching standards I would like our team to have. localized handling vs high level handling what level to log when logging understood exceptions.
		timcharperAuthorUnsubmitted Not Done Sadly we swallow errors. This is so severe that I think duplicating the exception is fine. If we see the exception at all we should treat it as a bug.
		kensipeUnsubmitted Not Done this example is very odd... creating an exception to log? (I get that it is thrown after but this is very odd)
		timcharperAuthorUnsubmitted Not Done It is true that it is odd. The advantage of creating an exception to log is that it will show a stack trace. There are other ways, but this is a really cheap way. My driving thought was this: "How can we know who is calling this method, so can change the code and make it stop". Unfortunately, since equals is a default-implemented java method, we cannot simply remove it and let the compiler point out where it is used. I wanted to be triple sure that: we noticed every usage every usage caused something to break in a very obvious way in the case that the breakage was not obvious (and an exception was swallowed), I wanted the exception to be in the logs. Is this excessive paranoia? Do you propose we do something else instead?
		jeschkiesUnsubmitted Not Done I'd be actually radical and crash Marathon in these cases. I'm fine with logging it for now. We don't have proper guidelines for logging and error handling yet anyways.
		timcharperAuthorUnsubmitted Not Done That's really not a bad idea, @jeschkies . :)
	45		throw ex
39	46		}
40	47
41	48		override def hashCode(): Int = lock(value.hashCode())
42	49
43	50		override def toString: String = lock {
44	51		value.toString
45	52		}
46	53		}
▲ Show 20 Lines • Show All 66 Lines • Show Last 20 Lines