Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: timer reset #51

Merged
merged 9 commits into from
May 17, 2022
Merged

fix: timer reset #51

merged 9 commits into from
May 17, 2022

Conversation

k-matsuzawa
Copy link
Collaborator

@k-matsuzawa k-matsuzawa commented May 15, 2022

Proposal

  • timer Reset処理の修正
    • timer.Reset実施箇所をtimer処理のgoroutineで行うように修正
    • Reset通知 、Timer通知 をそれぞれ独立したgoroutineで行うように修正
  • ログ周り強化
    • ステータス変更時
    • Timeout時
  • その他
    • ローカルのlint実行のgolangci-lint対応

@k-matsuzawa k-matsuzawa requested a review from fukushima-cg May 16, 2022 03:51
Copy link

@fukushima-cg fukushima-cg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

よさそうです

if !t.timer.Stop() {
select { // cleanup
case <-t.timer.C:
default:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default はあまり意味はないですか?
if !..Stop() 句で入っている時点で必ず timer.C から読み出せる?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

関連するIssue等を読み直してみましたが、確かに Stop()==false の時点で(かつTimerの受信側のgoroutineのselectで読み出しする場合)必ず読み出せるようです。(ややこしい、、)
golang/go#27169

コメントを残して不要な処理を削除するか、分かりやすさを重視してこのままで行くか、どちらがいいでしょう?(迷い中)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このままでいいかと思います。

select ... default 句は golang のイディオム的な意味合いも強そうなので、意図がわからないということもなさそうですので。

Copy link

@fukushima-cg fukushima-cg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@k-matsuzawa k-matsuzawa merged commit a3bc5ac into master May 17, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants