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

[Go] Fail to call Release on panic in Concatenate #36850

Closed
thorfour opened this issue Jul 24, 2023 · 0 comments · Fixed by #36854
Closed

[Go] Fail to call Release on panic in Concatenate #36850

thorfour opened this issue Jul 24, 2023 · 0 comments · Fixed by #36854
Assignees
Milestone

Comments

@thorfour
Copy link
Contributor

Describe the bug, including details regarding any error messages, version, and platform.

Today in the Concatenate function we recover if the concat function panics

defer func() {
if pErr := recover(); pErr != nil {
switch e := pErr.(type) {
case error:
err = fmt.Errorf("arrow/concat: %w", e)
default:
err = fmt.Errorf("arrow/concat: %v", pErr)
}
}
}()

However, if the concat function panics we don't call Release on the Array data that has been built up until the panic:

out := &Data{refCount: 1, dtype: data[0].DataType(), nulls: 0}

Which causes us to leak allocations in the memory.Allocator

Component(s)

Go

@thorfour thorfour changed the title [Go] Fail to call Release out panic in Concatenate [Go] Fail to call Release on panic in Concatenate Jul 24, 2023
zeroshade pushed a commit that referenced this issue Jul 25, 2023
…36854)

### Rationale for this change

The Concatenate function would capture panic's and return errors, however it wouldn't ensure that any allocations that happened in the `concat` sub function were cleaned up upon doing so. 

This change moves the `recover()` step into the `concat` function and will call `Release()` on the data object in the case of panic or error.  

### What changes are included in this PR?

### Are these changes tested?

A test called `TestConcatPanic` was added that causes the allocator to throw a panic part way during a concatenation, and ensures that the checked allocator still returns 0. 

### Are there any user-facing changes?

* Closes: #36850

Authored-by: thorfour <me@thor-hansen.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@zeroshade zeroshade added this to the 14.0.0 milestone Jul 25, 2023
R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this issue Aug 20, 2023
…ee'd (apache#36854)

### Rationale for this change

The Concatenate function would capture panic's and return errors, however it wouldn't ensure that any allocations that happened in the `concat` sub function were cleaned up upon doing so. 

This change moves the `recover()` step into the `concat` function and will call `Release()` on the data object in the case of panic or error.  

### What changes are included in this PR?

### Are these changes tested?

A test called `TestConcatPanic` was added that causes the allocator to throw a panic part way during a concatenation, and ensures that the checked allocator still returns 0. 

### Are there any user-facing changes?

* Closes: apache#36850

Authored-by: thorfour <me@thor-hansen.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…ee'd (apache#36854)

### Rationale for this change

The Concatenate function would capture panic's and return errors, however it wouldn't ensure that any allocations that happened in the `concat` sub function were cleaned up upon doing so. 

This change moves the `recover()` step into the `concat` function and will call `Release()` on the data object in the case of panic or error.  

### What changes are included in this PR?

### Are these changes tested?

A test called `TestConcatPanic` was added that causes the allocator to throw a panic part way during a concatenation, and ensures that the checked allocator still returns 0. 

### Are there any user-facing changes?

* Closes: apache#36850

Authored-by: thorfour <me@thor-hansen.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants