Refactor utilities under Foundation.Process and CartonHelpers.Process and share implementations #477

omochi · 2024-06-03T13:27:30Z

背景・課題

driver, plugin, test では Foundation.Process が使われています。
frontend, test では CartonHelpers(TSC).Process が使われています。

そして、driver と plugin と frontend の3箇所で、
それぞれ Process の拡張が行われています。

そのためプロジェクトを横断的に読み書きするのが難しくなっています。

同じことをやりたくても書き方が違ったり、
同じメソッド名なのに挙動が違ったりもするからです。
また、同じものが重複して実装されている部分もあります。

提案

このパッチではそういったユーティリティを整理して統一・共通化します。

`TSC.Process` の拡張については `CartonHelpers` モジュールに移動します。

TSC.Process と一緒に定義すれば良いからです。

`Foundation.Process` の拡張については、 `CartonCore` モジュールに移動します。

CartonCore はdriver, plugin の両方からアクセスできるモジュールとして最近定義されました。

`setSignalForwarding` の統一

これは TSC と Foundation の両方で局所的に書かれているので、
どちらもメソッドとして定義します。
ついでにバグがあったので直します。

`forwardTerminationSignals` の統一

setSignalForwarding を SIGINT と SIGTERM について設定する操作はよく出てくるので、
このメソッド名で統一します。

なお、元々はこれに terminationHandler による exit の転送が書いてありましたが、
これは削除しました。

waitUntilExit と一緒に使っていて意味がなかったからです。
waitUntilExit は terminationHandler の完了を待たないので、
そもそもレースコンディションが起きていました。
waitUntilExit の後で exit forward すれば十分で、その方が安定します。

`checkRun` の整理

Foudation.Process には static func な checkRun があります。
一方で、通常の run をさまざまな準備とともに使っている場所もあります。

この、さまざまな準備の処理は、ほとんど checkRun と被っています。

そこで、instance func な checkRun を実装してから、
static func な checkRun はそれを呼び出す形に書き換えます。

また、 checkRun の内部で使われているいくつかの処理は定型なので、
これもメソッドとして切り出します。

具体的には、 forwardExit と checkNonZeroExit を切り出しました。

シグナル転送の改修

従来、シグナル転送のロジックでは、
SIGINTとSIGTERMを受け取って、サブプロセスに SIGINT を送る実装になっていました。

ユーザはターミナルからSIGTERMを送ったはずなのに、
プロセスはSIGINTを受け取るということが起きてしまうのでこれは良くないと思います。

SIGINT を受け取ったら SIGINT を転送し、
SIGTERM を受け取ったら SIGTERM を転送するのが良いと思います。

ただし、 $ swift package plugin コマンドは、
SIGINT の転送をするが SIGTERM は転送しないようでした。
これは別途、swiftpmの実装を確認する予定です。

テストコードの中に carton driver に対して SIGTERM を送信するロジックがあり、
これが先述の転送挙動と組み合わさっていたために、
これを修正したところうまく終了できませんでした。

そのため、テストコードは SIGINT を送信するように修正しました。

omochi · 2024-06-04T00:23:43Z

このbytesの違いが出るやつは、 testDevServerPublish の手書きキャッシュが発動すると起きるんだけど、そんなバカな？

omochi · 2024-06-04T00:31:47Z

手元は通ったな CIのLinuxも通ったな CIのmacはなんだ？

omochi · 2024-06-04T03:15:32Z

Error: bind(descriptor:ptr:bytes:): Address already in use (errno: 48)

これがダメっぽいな。
おそらく exit(0) を転送するようにしたことで、
Swiftの defer が正常終了系で評価されなくなり、
サブプロセスの終了処理が無視され、
サーバが停止しなくなり、
ポートが使えなくなっている

omochi · 2024-06-04T03:18:20Z

エラーの場合も転送しようとしてexitするのは問題がありそうだ。

omochi · 2024-06-04T04:21:14Z

お、手元で同じエラーにできた

omochi · 2024-06-04T04:47:25Z

TERMの転送がうまくいってないな、直撃して死んでしまう

[omochi@omochi-mbp carton (org-procs *=)]$ pstree 60377
-+= 60377 omochi .build/arm64-apple-macosx/debug/carton dev --verbose --port 8080 --skip-auto-open
 \-+= 60387 omochi /Users/omochi/Library/Developer/Toolchains/swift-wasm-5.9.2-RELEASE.xctoolchain/usr/bin/swift-package --triple wasm32-unknow
   \-+= 60447 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/plugins/CartonDevPlugin/cache/CartonDevP
     \--= 60460 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/x86_64-apple-macosx/debug/carton-front
[omochi@omochi-mbp carton (org-procs *=)]$ kill -TERM 60377
[omochi@omochi-mbp carton (org-procs *=)]$ pstree 60377    
[omochi@omochi-mbp carton (org-procs *=)]$ pstree 60387 
-+= 60387 omochi /Users/omochi/Library/Developer/Toolchains/swift-wasm-5.9.2-RELEASE.xctoolchain/usr/bin/swift-package --triple wasm32-unknown-
 \-+= 60447 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/plugins/CartonDevPlugin/cache/CartonDevPlu
   \--= 60460 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/x86_64-apple-macosx/debug/carton-fronten
[omochi@omochi-mbp carton (org-procs *=)]$ kill 60387
[omochi@omochi-mbp carton (org-procs *=)]$ pstree 60387
[omochi@omochi-mbp carton (org-procs *=)]$ pstree 60447
-+= 60447 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/plugins/CartonDevPlugin/cache/CartonDevPlugin
 \--= 60460 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/x86_64-apple-macosx/debug/carton-frontend dev --main-
[omochi@omochi-mbp carton (org-procs *=)]$

omochi · 2024-06-04T04:49:38Z

あ！

omochi · 2024-06-04T04:56:31Z

SIGTERMを書くところを間違えてSIGKILLにしていた。
でもまだうまくいかない。

$ swift package コマンドは SIGTERM を転送しないっぽい？

[omochi@omochi-mbp carton (org-procs *=)]$ pstree 69794
-+= 69794 omochi .build/arm64-apple-macosx/debug/carton dev --verbose --port 8080 --skip-auto-open
 \-+= 69804 omochi /Users/omochi/Library/Developer/Toolchains/swift-wasm-5.9.2-RELEASE.xctoolchain/usr/bin/swift-package --triple wasm32-unknown-wasi --sc
   \-+= 69864 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/plugins/CartonDevPlugin/cache/CartonDevPlugin
     \--= 69877 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/x86_64-apple-macosx/debug/carton-frontend dev --m
[omochi@omochi-mbp carton (org-procs *=)]$ kill 69794
[omochi@omochi-mbp carton (org-procs *=)]$ pstree 69804
[omochi@omochi-mbp carton (org-procs *=)]$ pstree 69864
-+= 69864 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/plugins/CartonDevPlugin/cache/CartonDevPlugin
 \--= 69877 omochi /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/EchoExecutable/.build/carton/x86_64-apple-macosx/debug/carton-frontend dev --main-
[omochi@omochi-mbp carton (org-procs *=)]$

omochi

self comment

omochi · 2024-06-04T07:02:31Z

Sources/CartonCore/FoundationProcessEx.swift

+    let signalSource = DispatchSource.makeSignalSource(signal: signalNo)
+    signalSource.setEventHandler { [self] in
+      signalSource.cancel()
+      kill(processIdentifier, signalNo)


ここが元々 interrupt 固定になっていました。

omochi · 2024-06-04T07:03:39Z

Sources/CartonHelpers/ProcessEx.swift

@@ -17,3 +20,28 @@ extension ProcessResult {
    return try utf8Output()
  }
 }
+
+@discardableResult
+private func osSignal(


Process.signal メソッドと Darwin.signal が被るので、
リネームのためにラッパーを置きます。
ターゲットによって Darwin. じゃなかったりするため。

omochi · 2024-06-04T07:04:03Z

Sources/CartonHelpers/ProcessEx.swift

+    let signalSource = DispatchSource.makeSignalSource(signal: signalNo)
+    signalSource.setEventHandler {
+      signalSource.cancel()
+      self.signal(signalNo)


ここが元々 signal(SIGINT) 固定になっていました。

omochi · 2024-06-04T07:04:42Z

Tests/CartonCommandTests/DevCommandTests.swift

@@ -43,24 +43,23 @@ final class DevCommandTests: XCTestCase {
      // FIXME: Don't assume a specific port is available since it can be used by others or tests
      try await withFixture("EchoExecutable") { packageDirectory in
        let process = try swiftRunProcess(
-          ["carton", "dev", "--verbose", "--port", "8081", "--skip-auto-open"],
+          ["carton", "dev", "--verbose", "--port", "8080", "--skip-auto-open"],


ここなんですが、すぐ上の testWithNoArguments がサーバーを閉じられていない場合に、
8080 でぶつかってくれた方が問題を発見できるので、8081から変えたいです。

omochi · 2024-06-04T07:05:02Z

Tests/CartonCommandTests/DevCommandTests.swift


    // client delay... let the server start up
-    let delay: Duration = .seconds(30)


最初の1回で30秒待つのは手元の開発で不便すぎるので、
3秒にして試行回数を増やします。

omochi · 2024-06-04T07:05:18Z

Tests/CartonCommandTests/DevCommandTests.swift

@@ -78,7 +77,7 @@ final class DevCommandTests: XCTestCase {
  func checkForExpectedContent(process: SwiftRunProcess, at url: String) async throws {
    defer {
      // end the process regardless of success
-      process.process.signal(SIGTERM)
+      process.process.signal(SIGINT)


swiftpm pluginを正しく貫通する SIGINT を使います。

kateinoigakukun · 2024-06-04T10:06:07Z

Plugins/CartonBundlePlugin/CartonBundlePluginCommand.swift

+    try frontend.checkRun(
+      printsLoadingMessage: false,
+      didExit: {
+        print("Bundle written in \(bundleDirectory)")


It seems didExit is called regardless the process succeeded or failed, so the message is printed even though the frontend process failed.

Do we really need didExit hook point? It might be unnecessary if we move the message printing to the frontend

挙動を変えてしまっていますね、すみません。
何も考えずにロジックを維持しようとしましたが、たしかに先のコマンドが自分でメッセージを出せば良さそうです。やってみます。

omochi · 2024-06-04T12:20:59Z

Sources/CartonFrontend/Commands/CartonFrontendBundleCommand.swift

@@ -129,7 +129,7 @@ struct CartonFrontendBundleCommand: AsyncParsableCommand {
      topLevelResourcePaths: resources
    )

-    terminal.write("Bundle generation finished successfully\n", inColor: .green, bold: true)


このメッセージをfrontendで出した後で、plugin側で改めてパスを出していたため、以下のようなメッセージになっていました。

... Bundle generation finished successfully Bundle written in Bundle

これを統合し、frontend側で成功メッセージと配置したパスを表示するようにして、
plugin側のメッセージを出すのはやめました。
以下のような出力になります。

... Building for debugging... [386/386] Linking carton-frontend Build complete! (32.48s) Building "app" Right after building the main binary size is 7.67 MB After stripping debug info the main binary size is 5.49 MB Running... wasm-opt -Os --enable-bulk-memory /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/main.wasm -o /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/main.wasm `wasm-opt` process finished successfully After stripping debug info the main binary size is 3.49 MB Copying resources to /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/JavaScriptKit_JavaScriptKit.resources Copying resources to /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/DevServerTestApp_app.resources Creating symlink /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/style.css Bundle successfully generated at /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle

ファイルパスはフルで出るようにしました。

この修正に伴い、didExitコールバックは消しました。

omochi · 2024-06-04T12:22:06Z

Sources/CartonCore/FoundationProcessEx.swift

+    forwardExit: Bool = false
+  ) throws {
+    if printsLoadingMessage {
+      print("Running \(try commandLine)")


ここですが、元コードだと stderr に出力して flush していたのですが、
普通に Swift.print で良いと思いました。
frontendの他の部分でも進行状況メッセージは stdout に出ていますし。

omochi · 2024-06-04T12:22:39Z

@kateinoigakukun 指摘事項に対応しました

omochi · 2024-06-04T12:24:09Z

早すぎて入れ違いになってしまった。

omochi · 2024-06-04T12:28:18Z

SwiftPMが SIGINT を転送するが SIGTERM は転送しない問題はここの実装っぽい

https://github.com/apple/swift-package-manager/blob/09efb063a6ae7f1175e56d51e3affbf95c79e4ab/Sources/Basics/Cancellator.swift#L72

kill コマンドのデフォルトが SIGTERM だから対応してた方がいいと思うなあ

omochi added 2 commits June 3, 2024 22:25

organizer process utilities

81065e1

resurrect ProcessResult.checkNonZeroExit

27ec57a

omochi changed the title ~~[WIP] organizer process utilities~~ Foundation.ProcessとCartonHelpers.Processのユーティリティを整理し、共通化する Jun 4, 2024

import Foundation

f17179a

use SIGINT

301ea43

omochi commented Jun 4, 2024

View reviewed changes

omochi marked this pull request as ready for review June 4, 2024 07:05

kateinoigakukun reviewed Jun 4, 2024

View reviewed changes

remove didExit and tweak bundle completion message

333c6c8

omochi commented Jun 4, 2024

View reviewed changes

kateinoigakukun approved these changes Jun 4, 2024

View reviewed changes

omochi commented Jun 4, 2024

View reviewed changes

omochi requested a review from kateinoigakukun June 4, 2024 12:22

kateinoigakukun changed the title ~~Foundation.ProcessとCartonHelpers.Processのユーティリティを整理し、共通化する~~ Refactor utilities under Foundation.Process and CartonHelpers.Process and share implementations Jun 4, 2024

kateinoigakukun enabled auto-merge (squash) June 4, 2024 12:26

kateinoigakukun merged commit 924e171 into swiftwasm:main Jun 4, 2024
4 checks passed

omochi deleted the org-procs branch June 4, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor utilities under Foundation.Process and CartonHelpers.Process and share implementations #477

Refactor utilities under Foundation.Process and CartonHelpers.Process and share implementations #477

omochi commented Jun 3, 2024 •

edited

Loading

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi left a comment

omochi Jun 4, 2024

omochi Jun 4, 2024

omochi Jun 4, 2024

omochi Jun 4, 2024

omochi Jun 4, 2024

omochi Jun 4, 2024

kateinoigakukun Jun 4, 2024

omochi Jun 4, 2024

omochi Jun 4, 2024 •

edited

Loading

omochi Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024


		// client delay... let the server start up
		let delay: Duration = .seconds(30)

Refactor utilities under Foundation.Process and CartonHelpers.Process and share implementations #477

Refactor utilities under Foundation.Process and CartonHelpers.Process and share implementations #477

Conversation

omochi commented Jun 3, 2024 • edited Loading

背景・課題

提案

TSC.Process の拡張については CartonHelpers モジュールに移動します。

Foundation.Process の拡張については、 CartonCore モジュールに移動します。

setSignalForwarding の統一

forwardTerminationSignals の統一

checkRun の整理

シグナル転送の改修

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 4, 2024

omochi commented Jun 3, 2024 •

edited

Loading

`TSC.Process` の拡張については `CartonHelpers` モジュールに移動します。

`Foundation.Process` の拡張については、 `CartonCore` モジュールに移動します。

`setSignalForwarding` の統一

`forwardTerminationSignals` の統一

`checkRun` の整理

omochi Jun 4, 2024 •

edited

Loading