You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2023/11/20 16:46:22 UTC
[PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
zeroshade opened a new pull request, #38797:
URL: https://github.com/apache/arrow/pull/38797
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Adding RWMutex to protect `loc` in `TimestampType` and fix the race condition.
### Are these changes tested?
Yes, a unit test is added which is covered by the CI which runs with `-race`.
### Are there any user-facing changes?
Copying `TimestampType` will now be problematic and linters will show it as an error. In theory this shouldn't be a problem as most uses of TimestampType should be utilizing pointers to it rather than the value directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz merged PR #38797:
URL: https://github.com/apache/arrow/pull/38797
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38797:
URL: https://github.com/apache/arrow/pull/38797#issuecomment-1828399938
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 62e1e9ad76493729af6d605b7c0fcb31ebcbda5a.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/19067453527) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "benibus (via GitHub)" <gi...@apache.org>.
benibus commented on code in PR #38797:
URL: https://github.com/apache/arrow/pull/38797#discussion_r1399812967
##########
go/arrow/datatype_fixedwidth_test.go:
##########
@@ -180,6 +181,29 @@ func TestTimestampType_GetToTimeFunc(t *testing.T) {
assert.Equal(t, "2345-12-29T19:00:00-05:00", toTimeNY(ts).Format(time.RFC3339))
}
+func TestGetToTimeFuncRace(t *testing.T) {
Review Comment:
```suggestion
// Test race condition from GH-38795
func TestGetToTimeFuncRace(t *testing.T) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #38797:
URL: https://github.com/apache/arrow/pull/38797#discussion_r1400831750
##########
go/arrow/datatype_fixedwidth.go:
##########
@@ -398,10 +402,15 @@ func (t *TimestampType) ClearCachedLocation() {
// so if you change the value of TimeZone after calling this, make sure to call
// ClearCachedLocation.
func (t *TimestampType) GetZone() (*time.Location, error) {
+ t.mx.RLock()
if t.loc != nil {
+ defer t.mx.RUnlock()
return t.loc, nil
}
+ t.mx.RUnlock()
+ t.mx.Lock()
+ defer t.mx.Unlock()
Review Comment:
I'll add the check of `t.loc` after the write lock is acquired for now. My original attempt at this was to use `atomic.Pointer` for the loc but it made the test initialization for some of the tests annoying since you can't copy an `atomic.Pointer` and I didn't want to deal with refactoring the tests right now.
For now, the `RWMutex` should be good enough with the added check to avoid the duplicate work. Nice catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38797:
URL: https://github.com/apache/arrow/pull/38797#issuecomment-1819439818
:warning: GitHub issue #38795 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38797:
URL: https://github.com/apache/arrow/pull/38797#discussion_r1399886745
##########
go/arrow/datatype_fixedwidth.go:
##########
@@ -398,10 +402,15 @@ func (t *TimestampType) ClearCachedLocation() {
// so if you change the value of TimeZone after calling this, make sure to call
// ClearCachedLocation.
func (t *TimestampType) GetZone() (*time.Location, error) {
+ t.mx.RLock()
if t.loc != nil {
+ defer t.mx.RUnlock()
return t.loc, nil
}
+ t.mx.RUnlock()
+ t.mx.Lock()
+ defer t.mx.Unlock()
Review Comment:
Wait... :)
A full execution of `GetZone` is possible between `RUnlock()` and `Lock()`, so it's probably a good idea to check `t.loc` after the write lock is acquired.
If you want to improve the whole solution and make it mostly lock-free, you can combine atomic load/stores with a writers-only mutex. Porting this C++ snippet [1] to Go:
```cpp
std::atomic<Singleton*> Singleton::m_instance;
std::mutex Singleton::m_mutex;
Singleton* Singleton::getInstance() {
Singleton* tmp = m_instance.load();
if (tmp == nullptr) {
std::lock_guard<std::mutex> lock(m_mutex);
tmp = m_instance.load();
if (tmp == nullptr) {
tmp = new Singleton;
m_instance.store(tmp);
}
}
return tmp;
}
```
[1] https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/#using-c11-sequentially-consistent-atomics
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38797:
URL: https://github.com/apache/arrow/pull/38797#discussion_r1399886745
##########
go/arrow/datatype_fixedwidth.go:
##########
@@ -398,10 +402,15 @@ func (t *TimestampType) ClearCachedLocation() {
// so if you change the value of TimeZone after calling this, make sure to call
// ClearCachedLocation.
func (t *TimestampType) GetZone() (*time.Location, error) {
+ t.mx.RLock()
if t.loc != nil {
+ defer t.mx.RUnlock()
return t.loc, nil
}
+ t.mx.RUnlock()
+ t.mx.Lock()
+ defer t.mx.Unlock()
Review Comment:
Wait... :)
A full execution of `GetZone` is possible between `RUnlock()` and `Lock()`, so it's probably a good idea to check `t.loc` after the write lock is acquired.
For a lock-free way of doing this, you can combine atomic load/stores with a writers-only mutex. Porting this C++ snippet [1] to Go:
```cpp
std::atomic<Singleton*> Singleton::m_instance;
std::mutex Singleton::m_mutex;
Singleton* Singleton::getInstance() {
Singleton* tmp = m_instance.load();
if (tmp == nullptr) {
std::lock_guard<std::mutex> lock(m_mutex);
tmp = m_instance.load();
if (tmp == nullptr) {
tmp = new Singleton;
m_instance.store(tmp);
}
}
return tmp;
}
```
[1] https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/#using-c11-sequentially-consistent-atomics
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38795: [Go] Fix race GetToTimeFunc for Timestamp [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #38797:
URL: https://github.com/apache/arrow/pull/38797#discussion_r1406233011
##########
go/arrow/datatype_fixedwidth.go:
##########
@@ -398,10 +402,20 @@ func (t *TimestampType) ClearCachedLocation() {
// so if you change the value of TimeZone after calling this, make sure to call
// ClearCachedLocation.
func (t *TimestampType) GetZone() (*time.Location, error) {
+ t.mx.RLock()
if t.loc != nil {
+ defer t.mx.RUnlock()
return t.loc, nil
}
+ t.mx.RUnlock()
+ t.mx.Lock()
+ defer t.mx.Unlock()
+ // in case we got run in between releasing the read lock and
Review Comment:
Nit:
```suggestion
// in case GetZone() was called in between releasing the read lock and
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org