ARROW-2489: [Plasma] Fix PlasmaClient ABI variation #1933
Conversation
|
I think the AppVeyor failure is unrelated. It also succeeded on my AppVeyor account: https://ci.appveyor.com/project/pitrou/arrow/build/1.0.331 |
When compiled with GPU support, the PlasmaClient ABI would differ, leading to a crash in the Python bindings to Plasma.
94cb336 to
7d28354
Compare
| arrow::gpu::CudaDeviceManager* manager_; | ||
| #endif | ||
| class ARROW_NO_EXPORT Impl; | ||
| std::shared_ptr<Impl> impl_; |
There was a problem hiding this comment.
Shouldn't PIMPLs be at least a unique_ptr?
Note that we might should use something of the lines of http://brotherst-code.blogspot.de/2015/09/the-pimpl-idiom-youre-doing-it-wrong.html as we would still run into the problem of const correctness with PIMPLs otherwise.
There was a problem hiding this comment.
Oh, that's an interesting issue. We may want to open a JIRA about that.
As for the pointer kind: we will need shared_ptr to solve ARROW-2448. We can use unique_ptr in this PR and change it later if you prefer (but that'll probably change the ABI).
There was a problem hiding this comment.
Yeah, we will need the shared_ptr so I'm in favor of leaving it. I wonder if there is a good way to get the PIMPL const correctness with the shared pointer.
There was a problem hiding this comment.
For PlasmaClient I don't think const-ness is a very useful piece of information. It's a thing that talks over the network (what is a const TCP connection?).
There was a problem hiding this comment.
Yes, this was a more general piece of information. The const correctness is rather something that we would need with the core Arrow structures such as Array or Buffer.
pcmoritz
left a comment
There was a problem hiding this comment.
+1 Thanks for doing this!
| #include <assert.h> | ||
| #include <fcntl.h> | ||
| #include <netinet/in.h> | ||
| #include <stdbool.h> |
There was a problem hiding this comment.
We can probably remove this.
There was a problem hiding this comment.
Merging this now, I'll remove the include in #1939
This is a followup to #1933 which does reference counting of the PlasmaClient held by PlasmaBuffers to avoid the segfault in ARROW-2448. Author: Philipp Moritz <pcmoritz@gmail.com> Closes #1939 from pcmoritz/autoget-sharedptr and squashes the following commits: f1e6e8b <Philipp Moritz> fix test 2da395f <Philipp Moritz> update 13b1204 <Philipp Moritz> fixes b68b15e <Philipp Moritz> fix ObjectStatus 6d560db <Philipp Moritz> remove headers 94cdfd7 <Philipp Moritz> add test 6798ed0 <Philipp Moritz> Give shared_ptr of PlasmaClient::Impl to PlasmaBuffer
When compiled with GPU support, the PlasmaClient ABI would differ, leading to a crash in the Python bindings to Plasma.