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.

Sign up for GitHub

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

Jump to bottom

[Java] Fix memory leaks in disabled Flight tests #38318

Closed
jduo opened this issue Oct 17, 2023 · 1 comment · Fixed by #38719
Closed

[Java] Fix memory leaks in disabled Flight tests #38318

jduo opened this issue Oct 17, 2023 · 1 comment · Fixed by #38719
Assignees
@jduo
Labels
Component: Java Type: bug
Milestone
15.0.0

Comments

@jduo
Copy link
Member

jduo commented Oct 17, 2023

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

A few tests in the Arrow Flight packages are flaky and leak memory when closing the BufferAllocator.
See #6392 and #38268

These tests have been disabled but should be re-enabled once a fix is made.

The memory leak may be due to the FlightServer not completing its shutdown process. The call to FlightServer#close() only blocks for 6 seconds and may leak memory if that timeout gets exceeded.

Component(s)

Java
@jduo jduo added the Type: bug label Oct 17, 2023
@jduo jduo mentioned this issue Oct 17, 2023
Closed
@jduo
Copy link
Member Author

jduo commented Nov 14, 2023

take

jduo added a commit to Bit-Quill/arrow that referenced this issue Nov 14, 2023
@jduo
apacheGH-38318: [Java][FlightRPC] Enable tests that leaked
de7eeb2 
- Enable tests that were disabled due to flakey memory leaks
- Explicitly close child allocators in these tests to match
  the behavior of FlightServerTestRule which does not leak.
@github-actions github-actions bot mentioned this issue Nov 14, 2023
Merged
jduo added a commit to Bit-Quill/arrow that referenced this issue Nov 14, 2023
@jduo
apacheGH-38318: [Java][FlightRPC] Enable tests that leaked
5137755 
- Enable tests that were disabled due to flakey memory leaks
- Explicitly close child allocators in these tests to match
  the behavior of FlightServerTestRule which does not leak.
- Change TestBasicAuth to allocate only one server
- Change TestBasicAuth2 to allocate only one server and client
- Fix a bug in testAuth2#asyncPut() not including credentials
@jduo jduo mentioned this issue Nov 15, 2023
Open
lidavidm pushed a commit that referenced this issue Nov 15, 2023
@jduo
GH-38318: [Java][FlightRPC] Enable tests that leaked (#38719)
563078f 
### Rationale for this change
This enables tests that are currently disabled to improve coverage and help others build tests based on these.

### What changes are included in this PR?
- Enable tests that were disabled due to flakey memory leaks
- Explicitly close child allocators in these tests to match
  the behavior of FlightServerTestRule which does not leak.
- Change TestBasicAuth to allocate only one server
- Change TestBasicAuth2 to allocate only one server and client
- Fix a bug in testBasucAuth2#asyncPut() not including credentials

### Are these changes tested?
Tested locally.

### Are there any user-facing changes?
No.
* Closes: #38318

Authored-by: James Duong <james.duong@improving.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@lidavidm lidavidm changed the title Fix memory leaks in disabled Flight tests [Java] Fix memory leaks in disabled Flight tests Nov 15, 2023
@lidavidm lidavidm added this to the 15.0.0 milestone Nov 15, 2023
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@jduo @dgreiss
apacheGH-38318: [Java][FlightRPC] Enable tests that leaked (apache#38719
991c843 
)

### Rationale for this change
This enables tests that are currently disabled to improve coverage and help others build tests based on these.

### What changes are included in this PR?
- Enable tests that were disabled due to flakey memory leaks
- Explicitly close child allocators in these tests to match
  the behavior of FlightServerTestRule which does not leak.
- Change TestBasicAuth to allocate only one server
- Change TestBasicAuth2 to allocate only one server and client
- Fix a bug in testBasucAuth2#asyncPut() not including credentials

### Are these changes tested?
Tested locally.

### Are there any user-facing changes?
No.
* Closes: apache#38318

Authored-by: James Duong <james.duong@improving.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jduo jduo

Labels
Component: Java Type: bug
Projects
None yet
Milestone
15.0.0
Development

Successfully merging a pull request may close this issue.

GH-38318: [Java][FlightRPC] Enable tests that leaked Bit-Quill/arrow
2 participants
@lidavidm @jduo