Only bind one TestTty at a time (#873)

mosaic-tty-terminal/src/commonTest/kotlin/com/jakewharton/mosaic/tty/terminal/BaseEventParserTest.kt

@@ -7,7 +7,7 @@ import kotlin.test.AfterTest
 import kotlin.test.BeforeTest
 
 abstract class BaseEventParserTest {
-	internal val testTty = TestTty.create()
+	internal val testTty = TestTty.bind()
 	private val tty = testTty.tty
 	internal val parser = EventParser(tty)
 

mosaic-tty-terminal/src/commonTest/kotlin/com/jakewharton/mosaic/tty/terminal/TerminalTester.kt

@@ -23,7 +23,7 @@ import kotlinx.io.unsafe.UnsafeBufferOperations
 
 fun terminalTest(block: suspend TerminalTester.() -> Unit) {
 	runBlocking {
-		TestTty.create().use { testTty ->
+		TestTty.bind().use { testTty ->
 			TerminalTester(testTty).block()
 		}
 	}

mosaic-tty/api/mosaic-tty.api

@@ -1,8 +1,8 @@
 public final class com/jakewharton/mosaic/tty/TestTty : java/lang/AutoCloseable {
 	public static final field Companion Lcom/jakewharton/mosaic/tty/TestTty$Companion;
 	public synthetic fun <init> (JLcom/jakewharton/mosaic/tty/Tty;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public static final fun bind ()Lcom/jakewharton/mosaic/tty/TestTty;
 	public fun close ()V
-	public static final fun create ()Lcom/jakewharton/mosaic/tty/TestTty;
 	public final fun focusEvent (Z)V
 	public final fun getTty ()Lcom/jakewharton/mosaic/tty/Tty;
 	public final fun interruptRead ()V
@@ -14,7 +14,7 @@ public final class com/jakewharton/mosaic/tty/TestTty : java/lang/AutoCloseable
 }
 
 public final class com/jakewharton/mosaic/tty/TestTty$Companion {
-	public final fun create ()Lcom/jakewharton/mosaic/tty/TestTty;
+	public final fun bind ()Lcom/jakewharton/mosaic/tty/TestTty;
 }
 
 public final class com/jakewharton/mosaic/tty/Tty : java/lang/AutoCloseable {

mosaic-tty/api/mosaic-tty.klib.api

@@ -24,7 +24,7 @@ final class com.jakewharton.mosaic.tty/TestTty : kotlin/AutoCloseable { // com.j
     final fun write(kotlin/ByteArray, kotlin/Int, kotlin/Int): kotlin/Int // com.jakewharton.mosaic.tty/TestTty.write|write(kotlin.ByteArray;kotlin.Int;kotlin.Int){}[0]
 
     final object Companion { // com.jakewharton.mosaic.tty/TestTty.Companion|null[0]
-        final fun create(): com.jakewharton.mosaic.tty/TestTty // com.jakewharton.mosaic.tty/TestTty.Companion.create|create(){}[0]
+        final fun bind(): com.jakewharton.mosaic.tty/TestTty // com.jakewharton.mosaic.tty/TestTty.Companion.bind|bind(){}[0]
     }
 }
 

mosaic-tty/src/commonMain/c/mosaic-test-tty-posix.c

@@ -49,8 +49,9 @@ MosaicTestTtyInitResult testTty_init() {
 	}
 
 	MosaicTtyInitResult ttyInitResult = tty_initWithFd(childFd);
-	if (unlikely(ttyInitResult.error)) {
+	if (unlikely(!ttyInitResult.tty)) {
 		result.error = ttyInitResult.error;
+		result.already_bound = ttyInitResult.already_bound;
 		goto err_child;
 	}
 

mosaic-tty/src/commonMain/c/mosaic-test-tty-windows.c

@@ -14,11 +14,11 @@ typedef struct MosaicTestTtyImpl {
 	atomic_bool interrupt;
 } MosaicTestTtyImpl;
 
+static atomic_flag globalTestTty = ATOMIC_FLAG_INIT;
+
 MosaicTestTtyInitResult testTty_init() {
 	MosaicTestTtyInitResult result = {};
 
-	// TODO Atomic boolean guaranteeing single instance
-
 	MosaicTestTtyImpl *testTty = calloc(1, sizeof(MosaicTestTtyImpl));
 	if (unlikely(testTty == NULL)) {
 		// result.testTty is set to 0 which will trigger OOM.
@@ -46,10 +46,9 @@ MosaicTestTtyInitResult testTty_init() {
 	}
 
 	MosaicTtyInitResult ttyInitResult = tty_initWithHandles(conin, conoutWrite, true);
-	if (unlikely(ttyInitResult.error)) {
-		CloseHandle(conoutRead);
-		CloseHandle(conoutWrite);
+	if (unlikely(!ttyInitResult.tty)) {
 		result.error = ttyInitResult.error;
+		result.already_bound = ttyInitResult.already_bound;
 		goto err_conout;
 	}
 
@@ -59,6 +58,14 @@ MosaicTestTtyInitResult testTty_init() {
 
 	result.testTty = testTty;
 
+	if (unlikely(atomic_flag_test_and_set(&globalTestTty))) {
+		// We initialized an instance but there already was a global instance.
+		result.testTty = NULL;
+		result.error = tty_free(ttyInitResult.tty);
+		result.already_bound = true;
+		goto err_conout;
+	}
+
 	ret:
 	return result;
 
@@ -188,6 +195,7 @@ uint32_t testTty_free(MosaicTestTty *testTty) {
 		result = GetLastError();
 	}
 
+	atomic_flag_clear(&globalTestTty);
 	free(testTty);
 	return result;
 }

mosaic-tty/src/commonMain/c/mosaic-test-tty.h

@@ -10,6 +10,7 @@ typedef struct MosaicTestTtyImpl MosaicTestTty;
 typedef struct MosaicTestTtyInitResult {
 	MosaicTestTty *testTty;
 	uint32_t error;
+	bool already_bound;
 } MosaicTestTtyInitResult;
 
 MosaicTestTtyInitResult testTty_init();

mosaic-tty/src/commonMain/c/mosaic-tty-posix.c

@@ -14,6 +14,8 @@
 #include <time.h>
 #include <unistd.h>
 
+static _Atomic(MosaicTty *) globalTty;
+
 MosaicTtyInitResult tty_initWithFd(int fd) {
 	MosaicTtyInitResult result = {};
 
@@ -35,29 +37,6 @@ MosaicTtyInitResult tty_initWithFd(int fd) {
 
 	result.tty = tty;
 
-	ret:
-	return result;
-
-	err:
-	free(tty);
-	goto ret;
-}
-
-static _Atomic(MosaicTty *) globalTty;
-
-MosaicTtyInitResult tty_init() {
-	MosaicTtyInitResult result = {};
-
-	int fd = open("/dev/tty", O_RDWR);
-	if (unlikely(fd == -1)) {
-		result.error = errno;
-		result.no_tty = true;
-		goto ret;
-	}
-
-	result = tty_initWithFd(fd);
-
-	MosaicTty *tty = result.tty;
 	MosaicTty *expected = NULL;
 	if (likely(tty) && !atomic_compare_exchange_strong(&globalTty, &expected, tty)) {
 		// We initialized an instance but there already was a global instance.
@@ -68,6 +47,22 @@ MosaicTtyInitResult tty_init() {
 
 	ret:
 	return result;
+
+	err:
+	free(tty);
+	goto ret;
+}
+
+MosaicTtyInitResult tty_init() {
+	int fd = open("/dev/tty", O_RDWR);
+	if (likely(fd != -1)) {
+		return tty_initWithFd(fd);
+	}
+
+	MosaicTtyInitResult result = {};
+	result.error = errno;
+	result.no_tty = true;
+	return result;
 }
 
 void tty_setCallback(MosaicTty *tty, MosaicTtyCallback *callback) {
@@ -173,6 +168,8 @@ void sigwinchHandler(int value UNUSED) {
 		} else {
 			// TODO Send errno somewhere? Maybe once we get debug logs working.
 		}
+	} else {
+		// TODO Send warning somewhere? Maybe once we get debug logs working.
 	}
 }
 

mosaic-tty/src/commonMain/kotlin/com/jakewharton/mosaic/tty/TestTty.kt

@@ -2,7 +2,16 @@ package com.jakewharton.mosaic.tty
 
 public expect class TestTty : AutoCloseable {
 	public companion object {
-		public fun create(): TestTty
+
+		/**
+		 * Initialize a [TestTty] instance. Only a single [TestTty] instance can be bound at a time,
+		 * and only when a [Tty] is not also bound. Subsequent calls will throw an exception until
+		 * [TestTty.close] is called.
+		 *
+		 * @throws IOException If an error occurred creating the PTY.
+		 * @throws IllegalStateException If another instance is already bound.
+		 */
+		public fun bind(): TestTty
 	}
 
 	public val tty: Tty

mosaic-tty/src/commonTest/kotlin/com/jakewharton/mosaic/tty/TestTtyTest.kt

@@ -1,7 +1,10 @@
 package com.jakewharton.mosaic.tty
 
+import assertk.assertFailure
 import assertk.assertThat
+import assertk.assertions.hasMessage
 import assertk.assertions.isEqualTo
+import assertk.assertions.isInstanceOf
 import assertk.assertions.isZero
 import kotlin.test.AfterTest
 import kotlin.test.BeforeTest
@@ -13,7 +16,7 @@ import kotlinx.coroutines.launch
 import kotlinx.coroutines.test.runTest
 
 class TestTtyTest {
-	private val testTty = TestTty.create()
+	private val testTty = TestTty.bind()
 	private val tty = testTty.tty
 
 	@BeforeTest fun before() {
@@ -26,15 +29,11 @@ class TestTtyTest {
 		testTty.close()
 	}
 
-	@Test fun canCreateMultiple() {
-		if (isWindows()) return // TODO Not currently supported.
-
-		TestTty.create().use { testTty2 ->
-			testTty.write("hey\n")
-			testTty2.write("bye\n")
-			assertThat(testTty2.tty.read(4)).isEqualTo("bye\n")
-			assertThat(testTty.tty.read(4)).isEqualTo("hey\n")
-		}
+	@Test fun onlyOne() {
+		assertFailure {
+			TestTty.bind()
+		}.isInstanceOf<IllegalStateException>()
+			.hasMessage("TestTty or Tty already bound")
 	}
 
 	@Test fun multipleRawModeResetCycles() {

mosaic-tty/src/commonTest/kotlin/com/jakewharton/mosaic/tty/TtyTest.kt

@@ -18,7 +18,7 @@ import kotlinx.coroutines.test.runTest
 
 class TtyTest {
 	private val events = ArrayDeque<String>()
-	private val testTty = TestTty.create()
+	private val testTty = TestTty.bind()
 	private val tty = testTty.tty
 
 	@BeforeTest fun before() {

mosaic-tty/src/jvmMain/c/com_jakewharton_mosaic_tty_Jni.c

@@ -360,14 +360,20 @@ Java_com_jakewharton_mosaic_tty_Jni_testTtyInit(
 	jclass type UNUSED
 ) {
 	MosaicTestTtyInitResult result = testTty_init();
-	if (likely(!result.error)) {
+	if (likely(result.testTty)) {
 		return (jlong) result.testTty;
 	}
 
-	// This throw can fail, but the only condition that should cause that is OOM which
-	// will occur from returning 0 (which is otherwise ignored if the throw succeeds).
+	if (result.already_bound) {
+		jclass ise = (*env)->FindClass(env, "java/lang/IllegalStateException");
+		(*env)->ThrowNew(env, ise, "TestTty or Tty already bound");
+	} else if (result.error) {
 		throwIoe(env, result.error);
-	return 0;
+	} else {
+		jclass ise = (*env)->FindClass(env, "java/lang/OutOfMemoryException");
+		(*env)->ThrowNew(env, ise, NULL);
+	}
+	return 0; // Unused
 }
 
 JNIEXPORT jint JNICALL

mosaic-tty/src/jvmMain/kotlin/com/jakewharton/mosaic/tty/TestTty.kt

@@ -10,15 +10,12 @@ public actual class TestTty private constructor(
 	public actual companion object {
 		@JvmStatic
 		@Throws(IOException::class)
-		public actual fun create(): TestTty {
+		public actual fun bind(): TestTty {
 			val testTtyPtr = testTtyInit()
-			if (testTtyPtr != 0L) {
 			val ttyPtr = testTtyGetTty(testTtyPtr)
 			val tty = Tty(ttyPtr)
 			return TestTty(testTtyPtr, tty)
 		}
-			throw OutOfMemoryError()
-		}
 	}
 
 	@Throws(IOException::class)

mosaic-tty/src/nativeMain/kotlin/com/jakewharton/mosaic/tty/TestTty.kt

@@ -10,10 +10,13 @@ public actual class TestTty private constructor(
 	public actual val tty: Tty,
 ) : AutoCloseable {
 	public actual companion object {
-		public actual fun create(): TestTty {
+		public actual fun bind(): TestTty {
 			val testTtyPtr = testTty_init().useContents {
 				testTty?.let { return@useContents it }
 
+				if (already_bound) {
+					throw IllegalStateException("TestTty or Tty already bound")
+				}
 				if (error != 0U) {
 					throwIoe(error)
 				}