Skip to content

Commit d4e8fe3

Browse files
benjishultsbenjishults
authored
Merge pull request #29 from benjishults/semaphore
get rid of some race conditions in test helper
2 parents 9125d0d + 3e33a0b commit d4e8fe3

File tree

1 file changed

+125
-65
lines changed

1 file changed

+125
-65
lines changed

‎test/src/main/kotlin/bps/console/ComplexConsoleIoTestFixture.kt

Lines changed: 125 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
@file:Suppress("unused")
2+
13
package bps.console
24

35
import bps.console.app.MenuApplication
@@ -18,8 +20,7 @@ import kotlin.concurrent.thread
1820
/**
1921
* This fixture allows the application and the test validations to interact with each other in a thread-safe way.
2022
* The application is run in its own thread but it will pause between tests to allow the test code to validate
21-
* the outputs (and allowing the JDBC connection to remain open between tests). Using the [validateInteraction]
22-
* function will work for most use cases.
23+
* the outputs.
2324
*
2425
* Usage:
2526
*
@@ -71,6 +72,7 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
7172
fun validateInteraction(expectedOutputs: List<String>, toInput: List<String>) {
7273
require(toInput.isNotEmpty())
7374
outputs.clear()
75+
inputs.shouldBeEmpty()
7476
inputs.addAll(toInput)
7577
waitForApplicationProcessing()
7678
inputs.shouldBeEmpty()
@@ -145,7 +147,9 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
145147

146148
companion object {
147149

148-
enum class Owner {
150+
class ApplicationExitException(msg: String) : Exception(msg)
151+
152+
enum class AppStatus {
149153
/**
150154
* Indicates that the test is starting up and either thread might try to update data first.
151155
*/
@@ -154,12 +158,12 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
154158
/**
155159
* Indicates that the application thread owns the permit on [takeTurns]
156160
*/
157-
APP,
161+
RUNNING,
158162

159163
/**
160164
* Indicates that the application is waiting for input from the test.
161165
*/
162-
TEST,
166+
PAUSED,
163167

164168
/**
165169
* Indicates that the application thread has exited.
@@ -178,29 +182,52 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
178182
* Should only be set by the application thread while it has a permit on [takeTurns]
179183
*
180184
* INVARIANTS:
181-
* 1. This will only change values when the application thread owns the permit on [takeTurns]
182-
* 2. If the value is APP, then the application thread definitely owns a permit
183-
* 3. This is set to TEST when the application is waiting for input
185+
* 1. This will only change values when the application thread owns the permit on [takeTurns].
186+
* 2. If the value is [AppStatus.RUNNING], then the application thread definitely owns a permit.
187+
* 3. If the application is paused, then this will be [AppStatus.PAUSED].
188+
* 4. If the application is terminated, then this will be [AppStatus.APP_DONE].
189+
* 5. If the application has not yet tried to read input or produce output, then this will be [AppStatus.START_UP].
190+
*/
191+
@Volatile
192+
private var appStatus: AppStatus = AppStatus.START_UP
193+
194+
/**
195+
* INVARIANTS:
196+
* 1. If this is `true`, then the test has prepared some input and the application has not acknowledged it.
197+
* 2. If this is `false`, then the TEST is free to take action. The test should wait until this is `false` before taking back control unless [appStatus] is [AppStatus.APP_DONE]
198+
* 3. This value will only be changed by a thread that has a permit on [takeTurns]
199+
*
200+
* The application will set this to `false` when it wakes up from waiting for a permit on [takeTurns].
201+
*
202+
* The application will only change this value when [appStatus] is [AppStatus.RUNNING].
203+
*
204+
* The test will only change this value when [appStatus] is not [AppStatus.RUNNING].
184205
*/
185206
@Volatile
186-
private var owner: Owner = Owner.START_UP
207+
private var applicationHasUnackedTurnover: Boolean = false
187208

188209
/**
189210
* Ensure the application and test aren't messing each other up.
190211
*
191212
* Should never have more than one permit.
192213
*
193-
* When [owner] is [Owner.START_UP], the application cannot output until the test is ready. That
214+
* When [appStatus] is [AppStatus.START_UP], the application cannot output until the test is ready. That
194215
* readiness is indicated by the test creating a permit with a call to [Semaphore.release].
195216
*/
196217
// NOTE the test is expected to release (or create) a permit when it has inputs ready for the application
197218
// NOTE application should be careful not to call release unless it has a permit otherwise another permit will be created
198219
private val takeTurns = object : Semaphore(0, true) {
199-
override fun acquire() {
220+
221+
// fun applicationTryAquire(timeout: Long, unit: TimeUnit?): Boolean {
222+
// // NOTE this is a not-completely-effective way to try to catch cases when there are plural permits
223+
// return tryAcquire(timeout, unit)
224+
// }
225+
226+
override fun tryAcquire(timeout: Long, unit: TimeUnit?): Boolean {
200227
// NOTE this is a not-completely-effective way to try to catch cases when there are plural permits
201228
if (availablePermits() > 1)
202229
throw IllegalStateException("takeTurns must never have more than one permit")
203-
super.acquire()
230+
return super.tryAcquire(timeout, unit)
204231
}
205232

206233
override fun release() {
@@ -220,53 +247,67 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
220247
override val outputs = CopyOnWriteArrayList<String>()
221248

222249
override val inputReader = InputReader {
223-
if (owner === Owner.APP) {
224-
if (inputs.isNotEmpty())
225-
inputs.removeFirst()
226-
else {
227-
// application expected input that wasn't there
228-
owner = Owner.APP_DONE
229-
takeTurns.release()
230-
throw IllegalStateException("application was expecting more input than what was provided")
250+
when (appStatus) {
251+
AppStatus.RUNNING -> {
252+
if (inputs.isNotEmpty())
253+
inputs.removeFirst()
254+
else {
255+
// application expected input that wasn't there
256+
applicationHasUnackedTurnover = false
257+
appStatus = AppStatus.APP_DONE
258+
takeTurns.release()
259+
throw ApplicationExitException("application was expecting more input than what was provided")
260+
}
231261
}
232-
} else if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
233-
owner = Owner.APP_DONE
234-
throw IllegalStateException("Unable to acquire permit for application thread")
235-
} else {
236-
// NOTE at this point, the application thread has a permit and owns the data.
237-
owner = Owner.APP
238-
inputs.removeFirst()
262+
AppStatus.START_UP, AppStatus.PAUSED -> {
263+
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
264+
appStatus = AppStatus.APP_DONE
265+
throw ApplicationExitException("Unable to acquire permit for application thread within $awaitMillis milliseconds")
266+
} else {
267+
// NOTE at this point, the application thread has a permit and owns the data.
268+
appStatus = AppStatus.RUNNING
269+
inputs.removeFirst()
270+
}
271+
}
272+
AppStatus.APP_DONE -> throw ApplicationExitException("application was caught asking for input after reporting it had terminated")
239273
}
240274
}
241275

242276
// NOTE the application can only output when it owns a permit on [takeTurns]
243277
// NOTE when the inputs is empty, the application will release the permit, set the owner as TEST and wait for another permit
244278
// NOTE only the application calls this
279+
// TODO how about allow output when inputs is empty?
280+
// 1. the application would start editing outputs right away (may not be a problem as long as test doesn't check those until application is paused.)
281+
// 2. test would have to expect outputs in a different way
245282
override val outPrinter = OutPrinter {
246-
// FIXME there is probably a slight race condition here. On startup, the app might get to this code
247-
// at the same moment the test is turning over control. Can we say that only the app thread
248-
// can change the value of owner?
249-
if (owner !== Owner.APP) {
250-
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
251-
owner = Owner.APP_DONE
252-
throw IllegalStateException("Unable to acquire permit for application thread")
253-
} else {
254-
// NOTE at this point, the application thread has a permit and owns the data.
255-
owner = Owner.APP
283+
when (appStatus) {
284+
AppStatus.RUNNING -> {}
285+
AppStatus.START_UP, AppStatus.PAUSED, AppStatus.APP_DONE -> {
286+
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
287+
appStatus = AppStatus.APP_DONE
288+
throw ApplicationExitException("Unable to acquire permit for application thread within $awaitMillis milliseconds")
289+
} else {
290+
// NOTE at this point, the application thread has a permit and owns the data.
291+
appStatus = AppStatus.RUNNING
292+
}
256293
}
257294
}
258-
// NOTE at this point, the application thread has a permit and owns the data.
295+
// NOTE at this point, appStatus is RUNNING and the application thread has a permit.
296+
// NOTE pause if there are no inputs
259297
while (inputs.isEmpty()) {
260-
owner = Owner.TEST
261-
takeTurns.release()
262-
// NOTE the semaphore is fair so the test thread should pick it up.
263-
// NOTE we want to allow this to be interrupted by the test thread
264-
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
265-
owner = Owner.APP_DONE
266-
throw IllegalStateException("Unable to acquire permit for application thread")
267-
} else {
268-
owner = Owner.APP
269-
}
298+
// NOTE can there be a race condition here? I think not because the app thread owns a permit
299+
applicationHasUnackedTurnover = false
300+
do {
301+
appStatus = AppStatus.PAUSED
302+
takeTurns.release()
303+
// NOTE we want to allow this to be interrupted by the test thread
304+
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
305+
appStatus = AppStatus.APP_DONE
306+
throw ApplicationExitException("Unable to acquire permit for application thread within $awaitMillis milliseconds")
307+
} else {
308+
appStatus = AppStatus.RUNNING
309+
}
310+
} while (!applicationHasUnackedTurnover)
270311
}
271312
// NOTE at this point, the application thread has a permit and owns the data.
272313
outputs.add(it)
@@ -278,31 +319,45 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
278319
*/
279320
// NOTE only the test thread calls this
280321
override fun waitForApplicationProcessing() {
281-
when (owner) {
282-
Owner.TEST, Owner.START_UP -> {
283-
owner = Owner.APP
322+
when (appStatus) {
323+
AppStatus.PAUSED, AppStatus.START_UP -> {
324+
applicationHasUnackedTurnover = true
325+
// appStatus = AppStatus.RUNNING
284326
takeTurns.release()
285327
}
286-
Owner.APP_DONE -> fail("application exited unexpectedly")
287-
Owner.APP -> fail("test and application running simultaneously")
288-
}
289-
while (owner !== Owner.APP_DONE && outputs.isEmpty()) {
290-
println("Test waiting for application thread")
291-
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
328+
AppStatus.APP_DONE -> fail("application exited unexpectedly")
329+
AppStatus.RUNNING -> {
292330
stopApplication()
293-
fail("Unable to acquire permit from application thread")
331+
fail("test and application running simultaneously")
332+
}
333+
}
334+
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
335+
stopApplication()
336+
fail("Unable to acquire permit from application thread within $awaitMillis milliseconds")
337+
} else {
338+
// NOTE this thread has a permit
339+
while (appStatus !== AppStatus.APP_DONE && applicationHasUnackedTurnover) {
340+
// NOTE must be that the application hasn't picked up the permit, yet. We need to keep giving it chances.
341+
println("Test waiting for application thread to take a permit")
342+
takeTurns.release()
343+
if (!takeTurns.tryAcquire(awaitMillis, TimeUnit.MILLISECONDS)) {
344+
stopApplication()
345+
fail("Unable to acquire permit from application thread within $awaitMillis milliseconds")
346+
}
294347
}
295348
}
296349
}
297350

298351
override fun validateApplicationTerminated() {
299352
applicationThread.join(awaitMillis)
300-
applicationThread.state shouldBe Thread.State.TERMINATED
301-
owner shouldBe Owner.APP_DONE
353+
withClue("application thread should have terminated within $awaitMillis milliseconds") {
354+
applicationThread.state shouldBe Thread.State.TERMINATED
355+
}
356+
appStatus shouldBe AppStatus.APP_DONE
302357
}
303358

304359
override fun validateApplicationPaused() {
305-
owner shouldNotBe Owner.APP_DONE
360+
appStatus shouldNotBe AppStatus.APP_DONE
306361
applicationThread.state shouldNotBe Thread.State.TERMINATED
307362
}
308363

@@ -318,10 +373,12 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
318373
override fun close() {
319374
application.close()
320375
// NOTE immediately give control back to test
321-
val priorOwner = owner
322-
owner = Owner.APP_DONE
323-
if (priorOwner === Owner.APP) {
376+
if (appStatus == AppStatus.RUNNING || applicationHasUnackedTurnover) {
377+
appStatus = AppStatus.APP_DONE
378+
applicationHasUnackedTurnover = false
324379
takeTurns.release()
380+
} else {
381+
appStatus = AppStatus.APP_DONE
325382
}
326383
}
327384
}
@@ -335,7 +392,10 @@ interface ComplexConsoleIoTestFixture : SimpleConsoleIoTestFixture {
335392
// NOTE only test code calls this
336393
override fun stopApplication() {
337394
applicationThread.interrupt()
338-
applicationThread.join()
395+
applicationThread.join(awaitMillis)
396+
withClue("application thread should have terminated within $awaitMillis milliseconds") {
397+
applicationThread.state shouldBe Thread.State.TERMINATED
398+
}
339399
println("Application stopped")
340400
}
341401

0 commit comments

Comments
 (0)