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

fix coredump issue when client not successfully connected but need disconnect #28

Merged
merged 2 commits into from
Jul 30, 2021

Conversation

yma11
Copy link

@yma11 yma11 commented Jul 30, 2021

issue:

Core dump will happen when trying to client disconnect which actually not successfully created.

@github-actions
Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@yma11
Copy link
Author

yma11 commented Jul 30, 2021

@jikunshang Please take a review. Thanks.

@@ -35,13 +36,27 @@

private final long conn;

private PlasmaClientException plasmaClientException = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a flag rather than Exception?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated with using a flag instead.

protected void finalize() {
PlasmaClientJNI.disconnect(this.conn);
if (plasmaClientException == null) {
PlasmaClientJNI.disconnect(this.conn);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to solve the issue we met, but this may bring memory leak issue in cpp layer. We need figure out some way in later PR.

@jikunshang jikunshang merged commit cc49b68 into oap-project:arrow-4.0.0-oap-1.2 Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants