Skip to content

Kadai3 2/yuonoda #22

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

Closed
wants to merge 94 commits into from
Closed

Conversation

yuonoda
Copy link

@yuonoda yuonoda commented Jan 26, 2021

課題3-2「分割ダウンローダを作ろう」に回答しました。レビューなどいただけるとありがたいです。

テスト結果

% go test ./lib  --cover -v 
=== RUN   TestRun
=== RUN   TestRun/basic
2021/01/26 19:48:50 Run
2021/01/26 19:48:50 /Users/yuonoda/Downloads/jawiki-20210101-pages-articles-multistream-index.txt.bz2.download
2021/01/26 19:48:50 resource.getContent()
2021/01/26 19:48:50 resource.getSize()
2021/01/26 19:48:50 r.size: 25802009
2021/01/26 19:48:50 resource.getPartialContent(8600670, 17201339)
2021/01/26 19:48:50 resource.getPartialContent(0, 8600669)
2021/01/26 19:48:50 rangeVal[0]:bytes=8600670-17201339
2021/01/26 19:48:50 resource.getPartialContent(17201340, 25802009)
2021/01/26 19:48:50 rangeVal[0]:bytes=0-8600669
2021/01/26 19:48:50 rangeVal[0]:bytes=17201340-25802009
2021/01/26 19:48:51 res.StatusCode:206
2021/01/26 19:48:51 res.StatusCode:206
2021/01/26 19:48:51 res.StatusCode:206
2021/01/26 19:49:03 merging...
2021/01/26 19:49:03 pc.startByte: 8600670
2021/01/26 19:49:03 merging...
2021/01/26 19:49:03 pc.startByte: 17201340
2021/01/26 19:49:03 merging...
2021/01/26 19:49:03 pc.startByte: 0
2021/01/26 19:49:03 download succeeded!
--- PASS: TestRun (13.53s)
    --- PASS: TestRun/basic (13.53s)
=== RUN   TestFillByteArr
=== RUN   TestFillByteArr/basic
=== RUN   TestFillByteArr/basic2
--- PASS: TestFillByteArr (0.00s)
    --- PASS: TestFillByteArr/basic (0.00s)
    --- PASS: TestFillByteArr/basic2 (0.00s)
PASS
coverage: 76.5% of statements
ok      github.com/yuonoda/gopherdojo-studyroom/kadai3-2/yuonoda/lib    13.669s coverage: 76.5% of statements

yuonoda and others added 30 commits November 15, 2020 16:51
@gosagawa
Copy link

gosagawa commented Jan 30, 2021

実際に分割ダウンロードができるようになっており全般的に要件が満たせているかと思いますが、キャンセルが発生した時の実装を行うという要件については満たせていなさそうです。具体的に何をしなければいけないかというとCtrl+Cを押した時でも、ちゃんと途中で作成した一時ファイルとかを消さなくてはいけないという事になります。

さらっと書いてありますが、osのシグナルをゴルーチンで監視して、割り込みが入った時適切に処理をするという事を書かなくてはいけず、そこそこタフな実装になるかと思います。私が実際にこの課題をやった時も上手く実装できなかったです…。

キャンセル処理は資料のここに、
https://docs.google.com/presentation/d/1aVe4DLzSjjeppLm5RLW3EzSiEMn-u3OrssUGZSFCm7Q/edit#slide=id.g8ba4561d60_0_1345https://docs.google.com/presentation/d/1aVe4DLzSjjeppLm5RLW3EzSiEMn-u3OrssUGZSFCm7Q/edit#slide=id.g8ba4561d60_0_1345

キャンセル処理を満たせてそうな参考になりそうな回答は人のもので恐縮ですが以下が良さそうです
gopherdojo/dojo3#50

参考にしていただければと思います!

Comment on lines 149 to 169
// リクエスト回数分受け付けてマージ
var mu sync.Mutex
d.Content = make([]byte, d.Size)
for i := 0; i < d.BatchCount; i++ {
eg.Go(func() error {
select {
case <-ctx.Done():
return ctx.Err()
case pc := <-d.PartialContentCh:
mu.Lock()
utilities.FillByteArr(d.Content[:], pc.StartByte, pc.Body)
mu.Unlock()
}
return nil
})
}

// 1リクエストでも失敗すれば終了
if err := eg.Wait(); err != nil {
return err
}
Copy link
Author

Choose a reason for hiding this comment

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

このあたりを中心に、OSから中断シグナルがきたときに、リクエストを中止するようにしました。

@gosagawa
Copy link

gosagawa commented Feb 6, 2021

キャンセルもできるようになり、全般的によくなったかと思います!
すいません指摘が漏れていたのですが、一部エラーハンドリングの漏れがありそうです。
errcheckというツールがあり、それを使うと検出してくれます。

errcheck $(go list ./... )
downloader/downloader.go:112:22:        defer res.Body.Close()
downloader/downloader.go:146:25:        go d.GetPartialContent(startByte, endByte, ctx)
downloader/downloader.go:185:17:        defer os.Remove(dwFilePath)
downloader/downloader.go:198:11:        os.Rename(dwFilePath, finishedFilePath)

1,3番目はdeferなので無視したいところですが、実際には一応こんな書き方で回避できるし問題があった時確認ができます。

	defer func() {
		if err := res.Body.Close(); err != nil {
   		     log.Fatal(err)
		}
	}()

二番目はgoroutineで呼ぶ関数なので、errorを返す関数ではなく、
エラー用のチャネル

errCh := make(chan error)

を引数として渡してあげてエラーが発生した時このチャネルに送るようにした上で、後のセレクト部分を以下のようにすれば良さそうです。

			select {
			case <-ctx.Done():
				return ctx.Err()
			case err := <-errCh:
				return err
			case pc := <-d.PartialContentCh:
				mu.Lock()
				utilities.FillByteArr(d.Content[:], pc.StartByte, pc.Body)
				mu.Unlock()
			}

最後は単純なハンドリング漏れですね。

ここ以外は私が見た限りでは完成されているように思います!!

@yuonoda yuonoda closed this Feb 7, 2021
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

Successfully merging this pull request may close these issues.

2 participants