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

ZstdInputStreamNoFinalizer / ZstdOutputStreamNoFinalizer may leak buf… #303

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

normanmaurer
Copy link
Contributor

…fers when these are not arrray based.

Motivation:

We need to put the buffers back in the pool in case of validation failure as otherwise the buffer will leak.

Modifications:

Handle validation and buffer lifecycle via a static helper method

Result:

No more leak possible

…fers when these are not arrray based.

Motivation:

We need to put the buffers back in the pool in case of validation failure as otherwise the buffer will leak.

Modifications:

Handle validation and buffer lifecycle via a static helper method

Result:

No more leak possible
@normanmaurer
Copy link
Contributor Author

@luben something I noticed while reviewing some code.

@luben
Copy link
Owner

luben commented Mar 22, 2024

@normanmaurer
Copy link
Contributor Author

@luben fixed

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2024

Codecov Report

Merging #303 (fe1fc87) into master (20014d1) will increase coverage by 0.13%.
The diff coverage is 63.63%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #303      +/-   ##
============================================
+ Coverage     59.74%   59.87%   +0.13%     
  Complexity      307      307              
============================================
  Files            26       26              
  Lines          1473     1473              
  Branches        170      170              
============================================
+ Hits            880      882       +2     
+ Misses          436      435       -1     
+ Partials        157      156       -1     

@luben luben merged commit c66db1e into luben:master Mar 22, 2024
7 checks passed
@luben
Copy link
Owner

luben commented Mar 22, 2024

Thanks for the contribution!

@normanmaurer
Copy link
Contributor Author

Just noticed this was merged without squashing which makes the history a bit messy :/

@normanmaurer normanmaurer deleted the pool branch March 25, 2024 14:38
@luben
Copy link
Owner

luben commented Mar 25, 2024

:) not big deal

@dongjoon-hyun
Copy link
Contributor

Thank you all!

dongjoon-hyun added a commit to apache/orc that referenced this pull request Mar 28, 2024
### What changes were proposed in this pull request?

This PR aims to upgrade `zstd-jni` to 1.5.6-1.

### Why are the changes needed?

This release has the following memory-leak bug fix.

- https://github.com/luben/zstd-jni/releases/tag/v1.5.6-1
  - luben/zstd-jni#303

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #1865 from dongjoon-hyun/ORC-1670.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
dongjoon-hyun added a commit to apache/orc that referenced this pull request Mar 28, 2024
### What changes were proposed in this pull request?

This PR aims to upgrade `zstd-jni` to 1.5.6-1.

### Why are the changes needed?

This release has the following memory-leak bug fix.

- https://github.com/luben/zstd-jni/releases/tag/v1.5.6-1
  - luben/zstd-jni#303

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #1865 from dongjoon-hyun/ORC-1670.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 1830e86)
Signed-off-by: Dongjoon Hyun <[email protected]>
dongjoon-hyun added a commit to apache/spark that referenced this pull request Mar 28, 2024
### What changes were proposed in this pull request?

This PR aims to upgrade `zstd-jni` to 1.5.6-1.

### Why are the changes needed?

This release has the following memory-leak bug fix.

- https://github.com/luben/zstd-jni/releases/tag/v1.5.6-1
  - luben/zstd-jni#303

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #45756 from dongjoon-hyun/SPARK-47630.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
sweisdb pushed a commit to sweisdb/spark that referenced this pull request Apr 1, 2024
### What changes were proposed in this pull request?

This PR aims to upgrade `zstd-jni` to 1.5.6-1.

### Why are the changes needed?

This release has the following memory-leak bug fix.

- https://github.com/luben/zstd-jni/releases/tag/v1.5.6-1
  - luben/zstd-jni#303

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#45756 from dongjoon-hyun/SPARK-47630.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
szehon-ho pushed a commit to szehon-ho/spark that referenced this pull request Aug 7, 2024
This PR aims to upgrade `zstd-jni` to 1.5.6-1.

This release has the following memory-leak bug fix.

- https://github.com/luben/zstd-jni/releases/tag/v1.5.6-1
  - luben/zstd-jni#303

No.

Pass the CIs.

No.

Closes apache#45756 from dongjoon-hyun/SPARK-47630.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
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

Successfully merging this pull request may close these issues.

4 participants