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

ARROW-3348: [Plasma] Fix bug in which plasma store dies when object created by remo… #2650

Closed
wants to merge 5 commits into from

Conversation

@robertnishihara
Copy link
Contributor

robertnishihara commented Sep 28, 2018

…ved client is created.

This PR also includes a couple documentation and other very small changes which I'm happy to revert if people want.

…ved client is created.
// If the client is already disconnected, ignore release requests.
if (store_conn_ < 0) {
return Status::OK();
}

This comment has been minimized.

Copy link
@robertnishihara

robertnishihara Sep 28, 2018

Author Contributor

This change is unrelated to the bug.

@@ -584,6 +609,9 @@ void PlasmaStore::DisconnectClient(int client_fd) {
}
}

/// Remove all of the client's GetRequests.
RemoveGetRequestsForClient(client);

This comment has been minimized.

Copy link
@robertnishihara

robertnishihara Sep 28, 2018

Author Contributor

This is the key fix. Essentially, when a client disconnects, we need to clean up its get requests.

@@ -747,6 +748,35 @@ def test_use_one_memory_mapped_file(self):
create_object(self.plasma_client2, DEFAULT_PLASMA_STORE_MEMORY + 1,
0)

def test_client_death_during_get(self):

This comment has been minimized.

Copy link
@robertnishihara

robertnishihara Sep 28, 2018

Author Contributor

This test fails prior to this PR.

@@ -259,6 +259,46 @@ void PlasmaObject_init(PlasmaObject* object, ObjectTableEntry* entry) {
object->device_num = entry->device_num;
}

void PlasmaStore::RemoveGetRequest(GetRequest* get_request) {

This comment has been minimized.

Copy link
@robertnishihara

robertnishihara Sep 28, 2018

Author Contributor

This function is just factored out from PlasmaStore::ReturnFromGet. We shouldn't be using raw pointers.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 28, 2018

Codecov Report

Merging #2650 into master will increase coverage by 1.05%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2650      +/-   ##
==========================================
+ Coverage   87.19%   88.25%   +1.05%     
==========================================
  Files         381      319      -62     
  Lines       59223    55548    -3675     
==========================================
- Hits        51642    49025    -2617     
+ Misses       7507     6523     -984     
+ Partials       74        0      -74
Impacted Files Coverage Δ
cpp/src/plasma/store.h 100% <ø> (ø) ⬆️
cpp/src/plasma/client.cc 84.43% <100%> (ø) ⬆️
cpp/src/plasma/store.cc 91.05% <87.5%> (+1.15%) ⬆️
python/pyarrow/tests/test_plasma.py 96.32% <88.88%> (-0.29%) ⬇️
cpp/src/arrow/util/parsing.h 100% <0%> (ø) ⬆️
rust/src/record_batch.rs
go/arrow/datatype_nested.go
rust/src/util/bit_util.rs
go/arrow/math/uint64_amd64.go
go/arrow/internal/testing/tools/bool.go
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a503ef...09058e1. Read the comment docs.

get_requests.erase(it);
// If the vector is empty, remove the object ID from the map.
if (get_requests.empty()) {
object_get_requests_.erase(object_request_iter);

This comment has been minimized.

Copy link
@robertnishihara

robertnishihara Sep 29, 2018

Author Contributor

I added this line. I think this may have been a minor memory leak in the past, that is perhaps some keys never got removed from object_get_requests_.

@robertnishihara

This comment has been minimized.

Copy link
Contributor Author

robertnishihara commented Sep 29, 2018

Assuming tests pass, I will merge this unless others have comments.

Fix
Fix
@robertnishihara robertnishihara deleted the robertnishihara:plasmabug branch Sep 30, 2018
@wesm

This comment has been minimized.

Copy link
Member

wesm commented Sep 30, 2018

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.