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

Can I use a goldmark.Markdown from multiple goroutines? #182

Closed
karlmcguire opened this issue Jan 4, 2021 · 7 comments
Closed

Can I use a goldmark.Markdown from multiple goroutines? #182

karlmcguire opened this issue Jan 4, 2021 · 7 comments

Comments

@karlmcguire
Copy link

karlmcguire commented Jan 4, 2021

I've read all the documentation and haven't seen any mention of concurrency safety or the relative "cheapness" of producing a goldmark.Markdown for individual goroutines...

@karelbilek
Copy link
Contributor

I have done a simple test by changing DoTestCases and it seems like it's working fine.

// DoTestCases runs a set of test cases.
func DoTestCases(m goldmark.Markdown, cases []MarkdownTestCase, t TestingT, opts ...parser.ParseOption) {
	var wg sync.WaitGroup
	for _, testCase := range cases {
		testCase := testCase
		wg.Add(1)
		go func() {
			defer wg.Done()
			DoTestCase(m, testCase, t, opts...)
		}()
	}
	wg.Wait()
}

and go test ./... shows no error.

that's however not a guarantee.

And I should not commit that either, as the concurrency bugs would still be hard to replicate/debug if they actually happened. Maybe a separate test?

But I think it's all concurrency safe, looking at the code.

@karelbilek
Copy link
Contributor

So it's not trivially broken. :D But not sure how much has @yuin thought about concurrency either

@moorereason
Copy link
Contributor

Also, use go test -race ./... to enable the race detector.

@karelbilek
Copy link
Contributor

In a dumb way we can use the fuzz tests to check this too, but... not sure. As most of the fuzzer inputs are not markdown and are garbage (naturally), it will probably parse them as a single text block without much work :)

@karelbilek
Copy link
Contributor

@moorereason tried that too now, thx. Also no bugs.

@karelbilek
Copy link
Contributor

I tried this dumb fuzzing, not sure if I do it correctly, but again if there is something super broken it would catch it if I'm not wrong.

// Fuzz runs automated fuzzing against goldmark.
func Fuzz(data []byte) int {

	var b bytes.Buffer
	if err := markdown.Convert(data, &b); err != nil {
		return 0
	}
	correct := b.String()

	var wg sync.WaitGroup
	for i := 0; i < 100; i++ {
		wg.Add(1)
		go func() {
			var newb bytes.Buffer

			defer wg.Done()
			err := markdown.Convert(data, &newb)
			if err != nil {
				panic("foo")
			}

			second := newb.String()
			if correct != second {
				panic("incorrect")
			}
		}()
	}
	wg.Wait()

	return 1
}

It seems like it's working and not panicking. 🤷

@yuin
Copy link
Owner

yuin commented Jan 29, 2021

Yes, you can use goldmark.Markdown with multiple goroutines.

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

4 participants