maybe daily dev notes

私の開発日誌

トップ > AWS > AWS CDKのおもしろいバグを直した (Unhandled rejection編)

AWS CDKのおもしろいバグを直した (Unhandled rejection編)

AWS CDK 開発

先日自分が修正してマージされたAWS CDKのバグが個人的には結構面白かったので、ざっくりとまとめます。

github.com

どういうバグか

CDKのhotswapデプロイ*1をするとき、hotswapできそうでできない変更が2つ以上あると、CDKがクラッシュするという現象が起きていました。できそうでできない変更と言うと曖昧ですが、例えば下記のような変更です:

import * as lambda from 'aws-cdk-lib/aws-lambda';
import * as cf from 'aws-cdk-lib/aws-cloudfront';

// ...
    const distribution = new cf.Distribution(this, 'Distribution', {
      defaultBehavior: {
        origin: new S3Origin(bucket),
      },
    });

    new lambda.Function(this, 'Function1', {
      code: Code.fromInline('exports.handler=()=>{}'),
      runtime: Runtime.NODEJS_16_X,
      handler: 'index.handler',
      environment: {
        // ここを '2' に変えると、hotswapできそうでできない
        literal: '1', 
        // distribution.domainNameを含む環境変数はHotswap不可のため
        token: distribution.domainName,
    });

Lambdaの環境変数は基本的にはhotswap可能ですが、今回のように非対応のトークンを含む環境変数はhotswapできません。これを｢できそうでできない変更｣と呼んでいます (この記事だけの呼び方)。 より厳密には、hotswap可否の評価中に CfnEvaluationException などの例外を発生させる変更ということになりますが、細かい話なので単純化しておきます。

このような変更が2つ以上あるとCDKがクラッシュするというのが、今回のバグです。詳細な再現コードはこちら。

なぜこの現象が起きるか

問題の特定にはそれなりの時間を要しましたが、結論としてはNode.jsのUnhandled rejectionによる挙動でした。CDKではhotswap可否の評価をする際に、概ね以下のような処理を行っていました (実コード)。

  const promises: Array<Array<Promise<ChangeHotswapResult>>> = [];
  // 変更ごとにHotswap可否判定するPromiseを作成
  for (const 変更 of すべての変更のリスト) {
    const resourceHotswapEvaluation = isCandidateForHotswapping(change);

    if (リソースが確実にhotswapできないなら)
      continue
    } else { // hotswapできそうなら
      promises.push([
        isHotswappableLambdaFunctionChange(...),
        isHotswappableStateMachineChange(...),
        isHotswappableEcsServiceChange(...),
        // 他にも色々...
      ]);
    }
  }

  // 各Promiseの結果を見ていく
  const changesDetectionResults: Array<Array<ChangeHotswapResult>> = [];
  for (const detectorResultPromises of promises) {
    const hotswapDetectionResults = await Promise.all(detectorResultPromises);
    changesDetectionResults.push(hotswapDetectionResults);
  }

isHotswappable* 関数群はそれぞれPromiseを返す関数で、場合によっては例外を投げます (例えばHotswap未対応のトークンを含んでいる場合など)。

export async function isHotswappableLambdaFunctionChange(...): Promise<ChangeHotswapResult> {
  // 非同期処理
  if (...) {
    throw new CfnEvaluationException();
  }
}

ポイントは、1つ目のforループでpromiseの配列の配列を作成し、次のforループで各配列のpromiseをawaitしている点です。Unhandled rejectionは、Promise内で発生した例外をその時点で誰もcatchしなければ発生します。まさに handle されていないrejection (Promise内のエラー) というわけです。 具体的な発生条件は、次の記事に非常に詳しいです。 PromiseのUnhandled Rejectionを完全に理解する

今回のCDKコードを本質を損ねない範囲で簡略化すると、以下のようになります。まず最初のforループで、promises に2つのpromiseの配列を格納しています。それぞれのpromiseの中身はただ例外を投げる処理です。次のforループでは promises の各配列を await Promise.all します。この時、1つ目の配列をawaitした時点で例外が送出され、try/catch により大域脱出します。すると、2つ目の配列はawaitされません。つまり2つ目の配列で生じる例外は処理されないので、unhandled rejectionが起きるのです。

const throwError = async () => {
  throw new Error('Error!');
};

const main = async () => {
  const promises = [];
  try {
    // 最初のforループ　promiseの配列の配列を作成
    for (let i=0; i< 2; i++){
      promises.push([throwError()])
    }

    // 次のforループ　promiseの配列ごとに結果を取得
    for (const results of promises){
      await Promise.all(results);
    }
  } catch (e) {
    console.log(`gotcha ${e.message}`);
  }
};

main();

元のコードに戻りましょう。今回は promises.push でpromiseの配列の配列を作っていますが、この配列を作った時点でpromiseも作成されていることに注意してください。一方でこの promise たちが await されるのはしばらく後です。await されるまでに例外が発生した場合は、unhandled rejection が発生します。今回のクラッシュの根本原因もここでした。

変更が2つ以上という条件はここに絡んでいます。変更が1つだけであればpromisesの配列の長さは1なので、実質すぐに await されることとなり問題ありません。2つ以上で、かつ例外を投げる変更が2つ目以降に存在する場合に、この問題が生じるという仕組みだったのでした。

この因果関係を見出すのが苦労したポイントで、Unhandled rejection のことを全く知らないと難しかったのではと思います。頭の片隅に置いておいて良かったです。Node.js 16ではUnhandled rejectionが生じた場合、ただ例外とともにプロセスがクラッシュするので、一見何が起きているのか分かりづらいためです。これは --unhandled-rejections=warn というオプションを付けて実行することでマシになります。デバッグに役立つので覚えておくと良いでしょう。

$ node --unhandled-rejections=warn main.js
(node:16075) UnhandledPromiseRejectionWarning: Error: Error!
    at raiseErrorInMs (/Users/xxx/main.js:7:9)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:16075) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
gotcha Error!

どう修正するか

根本原因は分かりました。これを修正するには、いくつかの方法が考えられます。

1. promiseたちに catch を付けて、await せずとも例外が処理されるようにする

これは一つのやり方です。こうすれば、awaitする前に例外が投げられた場合でもcatchにより処理されるため、unhandled rejectionは生じません。しかしながら、実は今回は投げられた例外を大域脱出のために使っています (コード)。このため、例外をここでキャッチしてしまうのは不都合でした。大域脱出の仕組みを維持するには実装が複雑になりそうなので、この方法は使いません。

  promises.push([
     isHotswappableLambdaFunctionChange(...).catch(...), 
     isHotswappableStateMachineChange(...).catch(...),
     isHotswappableEcsServiceChange(...).catch(...),
  ]);

2. Promise.allSettled で promiseの配列をくるむ

Promise.allSettled を使うと、個々のpromiseはrejectされなくなります。代わりに、allSettledの戻り値で各promiseの結果を取得することができます (例)。1と同様に、個々の例外を明示的にhandleする必要がなくなるので、unhandled rejectionを防ぐことができますが、同じく大域脱出に対応するため追加のコードが必要になります。

  promises.push(Promise.allSettled([
     isHotswappableLambdaFunctionChange(...), 
     isHotswappableStateMachineChange(...),
     isHotswappableEcsServiceChange(...),
  ]));

3. promise たちを await する直前に作成する

await する前に例外が発生するのを避けたいのなら、awaitする直前にpromiseを作れば良いというアイデアです。この時 promises は、promiseの配列を返す関数の配列になります。その関数を await する時に呼べば、awaitされていないpromiseが裏で例外を投げる状況を防ぐことができます。

-  const promises: Array<Array<Promise<ChangeHotswapResult>>> = [];
+  const promises: Array<Array<()=>Promise<ChangeHotswapResult>>> = [];

-  promises.push([
+  promises.push(() => ([
     isHotswappableLambdaFunctionChange(...), 
     isHotswappableStateMachineChange(...),
     isHotswappableEcsServiceChange(...),
  ]);

  for (const detectorResultPromises of promises) {
-    const hotswapDetectionResults = await Promise.all(detectorResultPromises);
+    const hotswapDetectionResults = await Promise.all(detectorResultPromises());
    changesDetectionResults.push(hotswapDetectionResults);
  }

これは非常にシンプルでありながら、大域脱出の機能もそのまま維持されて良さそうです。1つ考えられる問題は、同時に実行される非同期関数が減ることで従来よりも並列度が下がり、処理時間が長くなりえることでした。これについては、コードを見ると実は isHotswappable* 関数は非同期関数でありながらほぼ非同期処理は行われていないため、async/await による速度向上効果は薄いと判断しています。今後の拡張で万が一速度面の問題が生じた場合に、また最適化を検討すれば十分でしょう。

上記の考察を経て、3番の実装でPRを投げ、無事マージされました。

このバグの元となった実装、知らないと結構やってしまいそうな気がします。単体テストで拾えるものでもないので、初回実装時に気づくのは難しそうです。現実的には有識者レビューでたまたま気づいてもらうか、気づかずにリリースして後から修正するかということになるんでしょうね〜。

まとめ

Unhandled rejectionについてはNode.js 14の時代に何度か遭遇し存在は知っていたのですが、今回始めて深堀りすることができました。こうした機会を得られるのもOSS貢献の良い側面だと考えているので、今後も続けていきたいと思います。

AWS CDKは最近謎の階級制 (下記) が導入されたりしてアツいOSSなので、興味を持った方はぜひ見てみてください。 CONTRIBUTING.md

github.com

*1:hotswapはLambdaやECS、Step Functionsなどの変更をすこぶる高速にデプロイできるCDKの目玉機能です！ぜひお試しください: cdk deploy --hotswap