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

Potential race condition with concurrent read/write #689

Closed
mlh758 opened this issue Aug 14, 2020 · 2 comments
Closed

Potential race condition with concurrent read/write #689

mlh758 opened this issue Aug 14, 2020 · 2 comments

Comments

@mlh758
Copy link
Contributor

mlh758 commented Aug 14, 2020

Description

I saw the announcement that this library now supports concurrent operations and this PR added an RWMutex to guard writes. It might be good to document the limitations of this.

Also it might be good to move the mutex into the File struct because this currently prevents writing to 2 different excel files concurrently, which was possible before.

Steps to reproduce the issue:

func TestConcurrency(t *testing.T) {
	f := NewFile()
	wg := new(sync.WaitGroup)
	for i := 0; i < 5; i++ {
		wg.Add(1)
		go func(val int) {
			f.SetCellValue("Sheet1", "A1", val)
			f.GetCellValue("Sheet1", "A1")
			wg.Done()
		}(i)
	}
	wg.Wait()
	val, err := f.GetCellValue("Sheet1", "A1")
	if err != nil {
		t.Error(err)
	}
	assert.Equal(t, "4", val)
}

Describe the results you received:

--- FAIL: TestConcurrency (0.01s)
    testing.go:906: race detected during execution of test
2020/08/14 12:24:00 xml decode error: XML syntax error on line 1: invalid UTF-8
2020/08/14 12:24:00 xml decode error: XML syntax error on line 1: invalid UTF-8
2020/08/14 12:24:01 xml decode error: XML syntax error on line 1: invalid UTF-8

Describe the results you expected:
Expected test to pass

Output of go version:

go version go1.14.2 darwin/amd64

Excelize version or commit ID:

32d066c677164c9

Environment details (OS, Microsoft Excel™ version, physical, etc.): MacOS

The locking also imposes some overhead for users who were using this without concurrency, or sticking to one file per thread:

Currently:

BenchmarkSetCellValue-12    	  260581	      4025 ns/op	    1938 B/op	      36 allocs/op

Version 2.2.0:

BenchmarkSetCellValue-12    	  336042	      3140 ns/op	    1922 B/op	      36 allocs/op

The overhead isn't too terrible, but we've had to deal with some performance issues in the past so a regression in that regard makes me a little uneasy. You might consider the potential for concurrent writes enough of a positive to offset the cost to write to a cell under normal conditions and if so feel free to disregard this part of the issue.

@xuri xuri closed this as completed in 55a5c5d Aug 15, 2020
xuri added a commit that referenced this issue Aug 15, 2020
xuri added a commit that referenced this issue Aug 15, 2020
xuri added a commit that referenced this issue Aug 15, 2020
xuri added a commit that referenced this issue Aug 15, 2020
author xuri <[email protected]> 1597421390 +0000
committer xuri <[email protected]> 1597481589 +0000

Compatible with Go 1.15, fix unit test failed on Windows and fixed #689 potential race condition
xuri added a commit that referenced this issue Aug 15, 2020
author xuri <[email protected]> 1597421390 +0000
committer xuri <[email protected]> 1597481589 +0000

Compatible with Go 1.15, fix unit test failed on Windows and fixed #689 potential race condition
xuri added a commit that referenced this issue Aug 15, 2020
@xuri
Copy link
Member

xuri commented Aug 15, 2020

@mlh758 Thanks for your feedback. I fixed race condition issues. Concurrent support will be impact normal r/w performance, slowdown about 20%, I will consider if or not support this feature later and optimize it. cc @hengmyj, @yinxiangjun.

@mlh758
Copy link
Contributor Author

mlh758 commented Aug 15, 2020

Sounds good.

One more comment I have - you're using an RWMutex but you're not using the Rlock and RUnlock methods on it at all to do read protection. It seems like you could use a regular Mutex if you don't need the mixed read/write optimizations provided by the RWMutex.

nullfy pushed a commit to nullfy/excelize that referenced this issue Oct 23, 2020
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

2 participants