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

Kadai3 2 kimuson13 #50

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions kadai3-2/kimuson13/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Binaries for programs and plugins
*.exe
*.exe~
*.dll
*.so
*.dylib

# Test binary, built with `go test -c`
*.test

# Output of the go coverage tool, specifically when used with LiteIDE
*.out

# Dependency directories (remove the comment below to include it)
# vendor/
17 changes: 17 additions & 0 deletions kadai3-2/kimuson13/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# 分割ダウンローダ
Rangeアクセスを用いて、ダウンロードする。
## 使用方法
まずはkimuson13のディレクトリに移動する。
その後、
```go run main.go``
もしくは、
```go build main.go```
```./main```
で実行可能。
## オプション
```-p <number>```
分割数を指定できる。
```-t <number of second>```
タイムアウトが起きる時間を指定できる。
```-f <file name>```
ダウンロード後のファイル名を指定できる。
19 changes: 19 additions & 0 deletions kadai3-2/kimuson13/TODO.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
TODO
===
- [x] GET requestを送ってみる
- [x] ダウンロードしたものをファイルにする
- [x] HEADでコンテンツのサイズを確認する
- [x] range requestの実装
- [x] 分割ダウンロードの実装
- [x] timeoutの実装

- [x] エラー処理を工夫する。(道場の資料39を参考にする。)
- [x] キャンセルが発生した場合の実装(道場の資料48を参考にする。)
- [x] 型とメソッドにまとめる
- [x] 入力された情報が正しいか判断する(govalidateのIsURLが使えそう。)

- [x] オプションのstructを作る(分割数、拡張子、タイムアウト、ダウンロードしたファイルの名前)
- [x] オプションに入力された引数が正しいか判断する。

- [x] 各関数の説明を書く
- [] テスト書く
217 changes: 217 additions & 0 deletions kadai3-2/kimuson13/download/download.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
package download

import (
"context"
"errors"
"fmt"
"io"
"net/http"
"os"
"time"

"golang.org/x/sync/errgroup"
)

// Donwnloader struct
type Downloader struct {
parallel int
timeout int
filename string
url string
}

// Rnage struct
type Range struct {
low int
high int
number int
}

// New for download package
func New(opts *Options) *Downloader {
return &Downloader{
parallel: opts.Parallel,
timeout: opts.Timeout,
filename: opts.Filename,
url: opts.URL,
}
}

// Run excecute method in download package
func (d *Downloader) Run(ctx context.Context) error {
if err := d.Preparate(); err != nil {
return err
}

contentLength, err := d.CheckContentLength(ctx)
if err != nil {
return err
}

Choose a reason for hiding this comment

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

Downloadでtempdirの作成、MergeFileでtempdirの削除を行なっていますが、エラーになったときにtmpdirが消されないケースがあるので、ここで以下のように作成の関数と削除の関数を作り削除の関数をdeferを使って呼ぶと良いです

if err := createTmpDir(); err != nil {
    return err
}
defer deleteTmpDir()

if err := d.Download(contentLength, ctx); err != nil {
return err
}

if err := d.MergeFile(d.parallel, contentLength); err != nil {
return err
}

return nil
}

//Preparate method define the variables to Donwload
func (d *Downloader) Preparate() error {

Choose a reason for hiding this comment

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

一応現状ソースのままならエラーが返ることは無いので、エラーを返す関数にする必要がないですね

if d.parallel < 1 {
d.parallel = 2

Choose a reason for hiding this comment

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

flagでデフォルト runtime.NumCPU()になっているのでそれに揃えるか、この関数自体をバリデーション用の関数として(名前もValidationとかに)、その上で1未満だとエラーを返すとかの方が良いのではと思います。

}

if d.timeout < 1 {
d.timeout = 5

Choose a reason for hiding this comment

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

ここもflagでは30sなので、そちらに揃える方が自然です。
かつ数字を直接書くよりは定数にしてflagの方もそれを使うのが良いでしょう。

}

return nil
}

// CheckContentLength method gets the Content-Length want to download
func (d *Downloader) CheckContentLength(ctx context.Context) (int, error) {
fmt.Fprintf(os.Stdout, "Start HEAD request to check Content-Length\n")

req, err := http.NewRequest("HEAD", d.url, nil)
if err != nil {
return 0, err
}
req = req.WithContext(ctx)

res, err := http.DefaultClient.Do(req)
if err != nil {
return 0, err
}

acceptRange := res.Header.Get("Accept-Ranges")
fmt.Fprintf(os.Stdout, "got: Accept-Ranges: %s\n", acceptRange)

Choose a reason for hiding this comment

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

Fprintfもerrorを返す巻数なので、エラー処理しておいた方が良いです

if acceptRange == "" {
return 0, errors.New("Accept-Range is not bytes")
}
if acceptRange != "bytes" {
return 0, errors.New("it is not content")
}

contentLength := int(res.ContentLength)
fmt.Fprintf(os.Stdout, "got: Content-Length: %v\n", contentLength)
if contentLength < 1 {
return 0, errors.New("it does not have Content-Length")
}

return contentLength, nil
}

// Download method does split-download with goroutine
func (d *Downloader) Download(contentLength int, ctx context.Context) error {
ctx, cancel := context.WithTimeout(ctx, time.Duration(d.timeout)*time.Second)
defer cancel()

if err := os.Mkdir("tempdir", 0755); err != nil {
return err
}

parallel := d.parallel
split := contentLength / parallel
grp, ctx := errgroup.WithContext(ctx)
for i := 0; i < parallel; i++ {
r := MakeRange(i, split, parallel, contentLength)
tempfile := fmt.Sprintf("tempdir/tempfile.%d.%d", parallel, r.number)
file, err := os.Create(tempfile)
if err != nil {
return err
}
defer file.Close()

Choose a reason for hiding this comment

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

L210でも指摘しましたが、ループでdeferは避けましょう

filename := file.Name()
grp.Go(func() error {
err := Requests(r, d.url, filename)
return err
})
}

if err := grp.Wait(); err != nil {
return err
}

return nil
}

// MakeRange function distributes Content-Length for split-download
func MakeRange(i, split, parallel, contentLength int) Range {
low := split * i
high := low + split - 1
if i == parallel-1 {
high = contentLength
}

return Range{
low: low,
high: high,
number: i,
}
}

// Requests function sends GET request
func Requests(r Range, url, filename string) error {
req, err := http.NewRequest("GET", url, nil)
if err != nil {
return err
}
req.Header.Set("Range", fmt.Sprintf("bytes=%d-%d", r.low, r.high))
fmt.Fprintf(os.Stdout, "start GET request: bytes=%d-%d\n", r.low, r.high)

res, err := http.DefaultClient.Do(req)
if err != nil {
return errors.New("error is here")
}
defer res.Body.Close()

if res.StatusCode != http.StatusPartialContent {
return fmt.Errorf("unexpected status code: %d", res.StatusCode)
}

output, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0666)
if err != nil {
return err
}
defer output.Close()

_, err = io.Copy(output, res.Body)
if err != nil {
return err
}
return nil
}

// MergeFile method merges tempfiles to new file
func (d *Downloader) MergeFile(parallel, contentLength int) error {
fmt.Println("\nmerging files...")
filename := d.filename
fh, err := os.Create(filename)
if err != nil {
return err
}
defer fh.Close()

Choose a reason for hiding this comment

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

エラー周りで複数指摘を入れていますが、これらを見つけるためにgithub.com/kisielk/errcheckというツールを使っています。

[~/go/src/github.com/kimuson13/gopherdojo-studyroom/kadai3-2/kimuson13] % errcheck ./...
download/download.go:77:13:     fmt.Fprintf(os.Stdout, "Start HEAD request to check Content-Length\n")
download/download.go:91:13:     fmt.Fprintf(os.Stdout, "got: Accept-Ranges: %s\n", acceptRange)
download/download.go:100:13:    fmt.Fprintf(os.Stdout, "got: Content-Length: %v\n", contentLength)
download/download.go:127:19:    defer file.Close()
download/download.go:164:13:    fmt.Fprintf(os.Stdout, "start GET request: bytes=%d-%d\n", r.low, r.high)
download/download.go:170:22:    defer res.Body.Close()
download/download.go:180:20:    defer output.Close()
download/download.go:197:16:    defer fh.Close()
download/download.go:210:12:    sub.Close()
download/option.go:23:11:       flg.Parse(args)

deferでのエラーハンドリングは少し工夫が必要な上、おきてもログに出すぐらいしかできないケースが多いのですが、それらに埋もれて本当にハンドルしなきゃいけないものが漏れるよりかは指摘がゼロになるように直した方が早いかと思うのでそうすることをお勧めします。

以下のような形で無名関数を使えば良いので参考にしてください。
https://joniy-joniy.hatenablog.com/entry/2017/12/31/182004


var n string
for i := 0; i < parallel; i++ {
n = fmt.Sprintf("tempdir/tempfile.%d.%d", parallel, i)
sub, err := os.Open(n)
if err != nil {
return err
}
_, err = io.Copy(fh, sub)
if err != nil {
return err
}
sub.Close()

Choose a reason for hiding this comment

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

ここでsub.Closeを呼ぶと上でエラーになったときにクローズされない可能性があります。そのため

		sub, err := os.Open(n)
		if err != nil {
			return err
		}
                 defer sub.Close()

このようにエラー処理の直後にdeferで閉じ処理をするまでが基本的な書き方です。

ただし今回のようにループでdeferを使うのは問題があるケースが多いです。最後にsubに対してループ回数分deferが実行されるので意図しない動作になります。なので、結論としてループの中の処理を関数に切り出してその中でdeferを使うのが適切かと思います。

}
if err := os.RemoveAll("tempdir"); err != nil {
return err
}
fmt.Println("complete parallel donwload!")
return nil
}
35 changes: 35 additions & 0 deletions kadai3-2/kimuson13/download/option.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package download

import (
"flag"
"net/url"
"runtime"
)

// Options struct
type Options struct {
Parallel int
Timeout int
Filename string
URL string
}

// Parse method parses options
func (opts *Options) Parse(args ...string) (*Options, error) {
flg := flag.NewFlagSet("parallelDownload", flag.ExitOnError)
parallel := flg.Int("p", runtime.NumCPU(), "separate Content-Length with this argument")
timeout := flg.Int("t", 30, "timeout for this second")
filename := flg.String("f", "paralleldownload", "save the file as this name")
flg.Parse(args)

Choose a reason for hiding this comment

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

失敗するケースを再現できなかったのですが、errorを返す関数であるのでエラー処理しておいた方が良さそうです。

u, err := url.Parse(flg.Arg(0))
if err != nil {
return nil, err
}

return &Options{
Parallel: *parallel,
Timeout: *timeout,
Filename: *filename,
URL: u.String(),
}, nil
}
59 changes: 59 additions & 0 deletions kadai3-2/kimuson13/download/option_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package download_test

import (
"runtime"
"testing"

"github.com/kimuson13/gopherdojo-studyroom/kimuson13/download"
)

var testurl string = "https://www.naoshima.net/wp-content/uploads/2019/07/393d0895747d5a947ad3acc35eb09688.pdf"

var options download.Options

var fileName string = "paralleldownload"

func TestParse(t *testing.T) {
cases := []struct {
name string
args []string
eParallel int
eTimeout int
eFilename string
}{
{name: "noOption", args: []string{testurl}, eParallel: runtime.NumCPU(), eTimeout: 30, eFilename: fileName},
{name: "parallelOption", args: []string{"-p=6", testurl}, eParallel: 6, eTimeout: 30, eFilename: fileName},
{name: "timeoutOption", args: []string{"-t=10", testurl}, eParallel: runtime.NumCPU(), eTimeout: 10, eFilename: fileName},
{name: "filenameOption", args: []string{"-f=test", testurl}, eParallel: runtime.NumCPU(), eTimeout: 30, eFilename: "test"},
{name: "PandT", args: []string{"-p=6", "-t=20", testurl}, eParallel: 6, eTimeout: 20, eFilename: fileName},
{name: "PandF", args: []string{"-p=6", "-f=test", testurl}, eParallel: 6, eTimeout: 30, eFilename: "test"},
{name: "TandF", args: []string{"-t=20", "-f=test", testurl}, eParallel: runtime.NumCPU(), eTimeout: 20, eFilename: "test"},
{name: "AllOption", args: []string{"-p=6", "-t=20", "-f=test", testurl}, eParallel: 6, eTimeout: 20, eFilename: "test"},
}
for _, c := range cases {
c := c
t.Run(c.name, func(t *testing.T) {
testParse(t, c.args, c.eParallel, c.eTimeout, c.eFilename)
})
}
}

func testParse(t *testing.T, args []string, parallel, timeout int, filename string) {
t.Helper()
opt, err := options.Parse(args...)
if err != nil {
t.Fatal(err)
}

if opt.Parallel != parallel {
t.Errorf("want %v, got %v", parallel, opt.Parallel)
}

if opt.Timeout != timeout {
t.Errorf("want %v, got %v", timeout, opt.Timeout)
}

if opt.Filename != filename {
t.Errorf("want %v, got %v", filename, opt.Filename)
}
}
5 changes: 5 additions & 0 deletions kadai3-2/kimuson13/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module github.com/kimuson13/gopherdojo-studyroom/kimuson13

go 1.16

require golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
2 changes: 2 additions & 0 deletions kadai3-2/kimuson13/go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ=
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
Loading