Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graceful.Server.Serve の引数に context.Context があるのは不適切では #10

Open
rmatsuoka opened this issue Nov 22, 2023 · 2 comments
Assignees

Comments

@rmatsuoka
Copy link
Contributor

rmatsuoka commented Nov 22, 2023

現在の Server の定義は次のようになっています。

type Server interface {
	Serve(ctx context.Context) error
	Shutdown(ctx context.Context) error
}

ここで、 問題なのはメソッド Serve の引数に context.Context があることです。これは不要というか無い方がいいのでは。
Server は「 Shutdown が呼ばれた時にシャットダウンする」のであって、引数の context がキャンセルされた時に停止するわけでは無いですよね。ここに context があると紛らわしいと思います。
単なる推測ですが、 (*http.Server).Serve や (*grpc.Server).Serve にも引数に context がないのもこれが理由なのではと思います。

@ne-sachirou
Copy link
Owner

context package - context - Go Packages#type Context ¶

	// If Done is not yet closed, Err returns nil.
	// If Done is closed, Err returns a non-nil error explaining why:
	// Canceled if the context was canceled
	// or DeadlineExceeded if the context's deadline passed.
	// After Err returns a non-nil error, successive calls to Err return the same error.

@ne-sachirou
Copy link
Owner

ne-sachirou commented Dec 5, 2023

TL;DR

  • Serve 函数は Context を受け付けなくていいでせう
    • ただし Serve 函数を timeout させたい用途が極めて稀であるといふ条件がつきます。この条件は満たされてゐると予想します
  • 常に書かなくてはならなくなり不便になる、共通の終了処理を、wrap する仕組みを用意します

  • document には、受け取った Context が cancel されたら context.Canceled を返せと書いてあります
  • 実用上はこの仕様を守って context.Canceled しても、現状のままで期待通りに動作します
    • 09d9967 の如し
    • これは Serve に失敗したか否かを判定する時に、Err が context.Canceled であれば Serve は成功したと見做す為です
      • Serve に成功したと見做すのは、NotifyContext で Context が Done した場合、Err は context.Canceled になる為に、この時を正常と扱ふのは必要な事です
      • Serve に失敗した時は、cancel する前に返された error を cancel(err) で Err に格納する為、判定を誤る事はありません
  • Serve に Context を渡すのは、この引数の Context が cancel したら Serve を中断する為と、Serve を中断するついでに shutdown してもよいと云ふ簡便な手段を提供する為の、2 つの理由があります
    • しかし Serve を中断するのは多くの場合で不要です
      • Serve の前処理で大きな副作用を実行する場合に限られます
      • Serve の中断で行ふ処理は、Shutdown で行ふ処理の一部に殆どの場合で一致します
        • つまり Shutdown を行へば、Serve を中断したのと同じ効果が殆どの場合で得られます
    • 簡便な手段は、別の簡便な手段を提供すれば置き換へられるかもしれません
  • Shutdown に Context を渡すのは timeout させる為です
    • Serve を timeout させたい可能性はあります
      • この case は熟考すべきです
      • 多くの場合は基盤環境からの readyness check でこの用は賄はれるでせう
    • Serve と Shutdown とが対称であるべき自明な理由はありません
  • 残る問題は、Shutdown で正しく終了する server を書くのは面倒であるといふ事です
    • d08a910 の如し
    • 面倒な終了前処理がある場合は、面倒でいいでせう。面倒な事をするのが面倒だといふのは、改善可能とはいへ納得できます
    • 特定の server では終了処理が無いといふ簡単な場合、簡単な事をするのが面倒だといふのは避けたいものです
      • しかし簡便であればよいのです。どうせ終了処理の枠組みはいつも同じなのですから、wrapper を提供すれば足りるでせう

@ne-sachirou ne-sachirou self-assigned this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants