From 7b29568eb19c211836ead0815927202f683f2791 Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Wed, 29 Jul 2015 12:15:02 +0100 Subject: [PATCH] Code cleanups. Fix more race conditions. --- middleware/markdown/markdown.go | 18 +++--- middleware/markdown/markdown_test.go | 59 +++++++++++++---- middleware/markdown/page.go | 96 ++++++++++++++++++++++------ middleware/markdown/process.go | 4 +- 4 files changed, 136 insertions(+), 41 deletions(-) diff --git a/middleware/markdown/markdown.go b/middleware/markdown/markdown.go index 7efa3db7b..03f4e077a 100644 --- a/middleware/markdown/markdown.go +++ b/middleware/markdown/markdown.go @@ -4,13 +4,14 @@ package markdown import ( "io/ioutil" + "log" "net/http" "os" "strings" + "sync" "github.com/mholt/caddy/middleware" "github.com/russross/blackfriday" - // "log" ) // Markdown implements a layer of middleware that serves @@ -70,11 +71,14 @@ type Config struct { // Directory to store static files StaticDir string + + sync.RWMutex } // ServeHTTP implements the http.Handler interface. func (md Markdown) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { - for _, m := range md.Configs { + for i := range md.Configs { + m := &md.Configs[i] if !middleware.Path(r.URL.Path).Matches(m.PathScope) { continue } @@ -120,11 +124,9 @@ func (md Markdown) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error if m.StaticDir != "" { // Markdown modified or new. Update links. - // go func() { - // if err := GenerateLinks(md, &md.Configs[i]); err != nil { - // log.Println(err) - // } - // }() + if err := GenerateLinks(md, m); err != nil { + log.Println(err) + } } body, err := ioutil.ReadAll(f) @@ -137,7 +139,7 @@ func (md Markdown) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error Req: r, URL: r.URL, } - html, err := md.Process(m, fpath, body, ctx) + html, err := md.Process(*m, fpath, body, ctx) if err != nil { return http.StatusInternalServerError, err } diff --git a/middleware/markdown/markdown_test.go b/middleware/markdown/markdown_test.go index e01d88e8e..28dd22848 100644 --- a/middleware/markdown/markdown_test.go +++ b/middleware/markdown/markdown_test.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "testing" "github.com/mholt/caddy/middleware" @@ -18,20 +19,24 @@ func TestMarkdown(t *testing.T) { FileSys: http.Dir("./testdata"), Configs: []Config{ Config{ - Renderer: blackfriday.HtmlRenderer(0, "", ""), - PathScope: "/blog", - Extensions: []string{".md"}, - Styles: []string{}, - Scripts: []string{}, - Templates: templates, + Renderer: blackfriday.HtmlRenderer(0, "", ""), + PathScope: "/blog", + Extensions: []string{".md"}, + Styles: []string{}, + Scripts: []string{}, + Templates: templates, + StaticDir: DefaultStaticDir, + StaticFiles: make(map[string]string), }, Config{ - Renderer: blackfriday.HtmlRenderer(0, "", ""), - PathScope: "/log", - Extensions: []string{".md"}, - Styles: []string{"/resources/css/log.css", "/resources/css/default.css"}, - Scripts: []string{"/resources/js/log.js", "/resources/js/default.js"}, - Templates: make(map[string]string), + Renderer: blackfriday.HtmlRenderer(0, "", ""), + PathScope: "/log", + Extensions: []string{".md"}, + Styles: []string{"/resources/css/log.css", "/resources/css/default.css"}, + Scripts: []string{"/resources/js/log.js", "/resources/js/default.js"}, + Templates: make(map[string]string), + StaticDir: DefaultStaticDir, + StaticFiles: make(map[string]string), }, }, IndexFiles: []string{"index.html"}, @@ -123,4 +128,34 @@ func getTrue() bool { if respBody != expectedBody { t.Fatalf("Expected body: %v got: %v", expectedBody, respBody) } + + expectedLinks := []string{ + "/blog/test.md", + "/log/test.md", + } + + for i, c := range md.Configs { + if c.Links[0].Url != expectedLinks[i] { + t.Fatalf("Expected %v got %v", expectedLinks[i], c.Links[0].Url) + } + } + + // attempt to trigger race condition + var w sync.WaitGroup + f := func() { + req, err := http.NewRequest("GET", "/log/test.md", nil) + if err != nil { + t.Fatalf("Could not create HTTP request: %v", err) + } + rec := httptest.NewRecorder() + + md.ServeHTTP(rec, req) + w.Done() + } + for i := 0; i < 5; i++ { + w.Add(1) + go f() + } + w.Wait() + } diff --git a/middleware/markdown/page.go b/middleware/markdown/page.go index c1c82dcb2..73a4f4557 100644 --- a/middleware/markdown/page.go +++ b/middleware/markdown/page.go @@ -12,17 +12,15 @@ import ( "github.com/russross/blackfriday" ) -var ( - pagesMutex sync.RWMutex - linksGenerating bool -) - const ( + // Date format YYYY-MM-DD HH:MM:SS timeLayout = `2006-01-02 15:04:05` + + // Length of page summary. summaryLen = 150 ) -// Page represents a statically generated markdown page. +// PageLink represents a statically generated markdown page. type PageLink struct { Title string Summary string @@ -30,25 +28,51 @@ type PageLink struct { Url string } -// pageLinkSorter sort PageLink by newest date to oldest. +// pageLinkSorter sorts PageLink by newest date to oldest. type pageLinkSorter []PageLink func (p pageLinkSorter) Len() int { return len(p) } func (p pageLinkSorter) Swap(i, j int) { p[i], p[j] = p[j], p[i] } func (p pageLinkSorter) Less(i, j int) bool { return p[i].Date.After(p[j].Date) } -func GenerateLinks(md Markdown, cfg *Config) error { - if linksGenerating { - return nil - } +type linkGen struct { + generating bool + waiters int + lastErr error + sync.RWMutex + sync.WaitGroup +} - pagesMutex.Lock() - linksGenerating = true +func (l *linkGen) addWaiter() { + l.WaitGroup.Add(1) + l.waiters++ +} + +func (l *linkGen) discardWaiters() { + l.Lock() + defer l.Unlock() + for i := 0; i < l.waiters; i++ { + l.Done() + } +} + +func (l *linkGen) started() bool { + l.RLock() + defer l.RUnlock() + return l.generating +} + +func (l *linkGen) generateLinks(md Markdown, cfg *Config) { + l.Lock() + l.generating = true + l.Unlock() fp := filepath.Join(md.Root, cfg.PathScope) cfg.Links = []PageLink{} - err := filepath.Walk(fp, func(path string, info os.FileInfo, err error) error { + + cfg.Lock() + l.lastErr = filepath.Walk(fp, func(path string, info os.FileInfo, err error) error { for _, ext := range cfg.Extensions { if !info.IsDir() && strings.HasSuffix(info.Name(), ext) { // Load the file @@ -92,12 +116,46 @@ func GenerateLinks(md Markdown, cfg *Config) error { } } - // sort by newest date - sort.Sort(pageLinkSorter(cfg.Links)) return nil }) - linksGenerating = false - pagesMutex.Unlock() - return err + // sort by newest date + sort.Sort(pageLinkSorter(cfg.Links)) + cfg.Unlock() + + l.Lock() + l.generating = false + l.Unlock() +} + +type linkGenerator struct { + gens map[*Config]*linkGen + sync.Mutex +} + +var generator = linkGenerator{gens: make(map[*Config]*linkGen)} + +// GenerateLinks generates links to all markdown files ordered by newest date. +// This blocks until link generation is done. When called by multiple goroutines, +// the first caller starts the generation and others only wait. +func GenerateLinks(md Markdown, cfg *Config) error { + generator.Lock() + + // if link generator exists for config and running, wait. + if g, ok := generator.gens[cfg]; ok { + if g.started() { + g.addWaiter() + generator.Unlock() + g.Wait() + return g.lastErr + } + } + + g := &linkGen{} + generator.gens[cfg] = g + generator.Unlock() + + g.generateLinks(md, cfg) + g.discardWaiters() + return g.lastErr } diff --git a/middleware/markdown/process.go b/middleware/markdown/process.go index 64ca313c2..8afd92b7b 100644 --- a/middleware/markdown/process.go +++ b/middleware/markdown/process.go @@ -101,9 +101,9 @@ func (md Markdown) processTemplate(c Config, requestPath string, tmpl []byte, me Links: c.Links, } - pagesMutex.RLock() + c.RLock() err = t.Execute(b, mdData) - pagesMutex.RUnlock() + c.RUnlock() if err != nil { return nil, err