ARROW-4583: [Plasma] Fix some small bugs reported by code scan tool #3656

guoyuhong · 2019-02-15T12:03:58Z

We used a static code scan tool to scan arrow code. There are several possible bugs:

The return value of PyDict_SetItem is not used.
Currently, EventLoop:: Shutdown should be called explicitly, which is error-prone and causing leak when the user forgets to call it.
There is an unclosed file descriptor in io.cc when path name is too long.

Besides, we also made the following small changes:

When we use Plasma in Yarn and when a node uses too much memory, a SIGTERM signal will be sent to Plasma. Current plasma will exit silently. We also some log to plasma store to help us to debug.
ARROW_LOG will always evaluate the output expression even when it is not enabled, which is not efficient.
The constructor of Java class ObjectStoreData is private, which is not convenient when we want to create a mock plasma store.
Fix a call to ObjectStoreData which misplaces meta and data according to https://github.com/apache/arrow/blob/master/java/plasma/src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java#L32 .

xhochy · 2019-02-15T13:32:00Z

@guoyuhong Out of curiosity which "code scan tool" did you use?

guoyuhong · 2019-02-15T15:07:49Z

@xhochy It is called Coverity.

wesm · 2019-02-15T15:49:39Z

Coverity just started offering free scans to open source projects again. @pitrou can you review?

guoyuhong · 2019-02-15T16:50:19Z

cpp/src/arrow/util/logging.h

There are lint tool conflicts here, so I disable clang-format.

guoyuhong · 2019-02-16T07:51:23Z

@pcmoritz Please take a look at this PR.

guoyuhong · 2019-02-18T02:58:29Z

Ping... @pcmoritz

pitrou

Thanks for doing this @guoyuhong . Here are some comments.

cpp/src/arrow/python/deserialize.cc

cpp/src/plasma/store.cc

cpp/src/arrow/util/logging.h

pitrou · 2019-02-18T16:43:04Z

cpp/src/arrow/python/deserialize.cc

Ah, sorry for not being specific here. It's better to not override the second parameter, so that the proper status code is returned (for example a Python MemoryError would be turned into a OutOfMemory Status, etc.).

(attempting to fix it directly from Github...)

Thanks! I checked out common.h. Yes, ConvertPyError() is better.

pitrou · 2019-02-18T18:30:02Z

Rebased, will wait for CI.

pitrou · 2019-02-18T21:44:59Z

Hmm, I really don't understand what the merge script is saying me here:

$ ./dev/merge_arrow_pr.py 
ARROW_HOME = /home/antoine/arrow
PROJECT_NAME = arrow
Which pull request would you like to merge? (e.g. 34): 3656

Pull request 3656 is not mergeable in its current form.
Continue? (experts only!) (y/n): n

@wesm did something weird happen on git master?

wesm · 2019-02-19T00:39:39Z

@pitrou master had a hiccup about 5 hours ago at 19:30 UTC; I accidentally created a merge commit in master (using the GitHub UI -- I have since asked INFRA to disallow merge commits through the UI) and quickly rebased and force-pushed master. It's possible that this PR was impacted. I have also seen the merge script misbehave at times like this when the GitHub API returns bad results now and then

kszucs · 2019-02-19T01:00:02Z

@wesm I've rebased it since that.

pitrou · 2019-02-19T11:07:13Z

Finally merged. Thanks @guoyuhong !

guoyuhong · 2019-02-19T11:30:37Z

@pitrou Thanks!

We used a static code scan tool to scan arrow code. There are several possible bugs: 1. The return value of `PyDict_SetItem` is not used. 2. Currently, `EventLoop:: Shutdown` should be called explicitly, which is error-prone and causing leak when the user forgets to call it. 3. There is an unclosed file descriptor in `io.cc` when path name is too long. Besides, we also made the following small changes: 1. When we use Plasma in Yarn and when a node uses too much memory, a SIGTERM signal will be sent to Plasma. Current plasma will exit silently. We also some log to plasma store to help us to debug. 2. `ARROW_LOG` will always evaluate the output expression even when it is not enabled, which is not efficient. 3. The constructor of Java class `ObjectStoreData` is private, which is not convenient when we want to create a mock plasma store. 4. Fix a call to `ObjectStoreData` which misplaces `meta` and `data` according to https://github.com/apache/arrow/blob/master/java/plasma/src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java#L32 . Author: Yuhong Guo <[email protected]> Author: Antoine Pitrou <[email protected]> Closes apache#3656 from guoyuhong/fixPlasma and squashes the following commits: 634e36a <Antoine Pitrou> Use default argument value to `ConvertPyError` b547f2f <Yuhong Guo> remove if from ARROW_LOG 440f097 <Yuhong Guo> Address comment d3eb22f <Yuhong Guo> Lint 79b4af3 <Yuhong Guo> Fix and Lint 434a039 <Yuhong Guo> Make constructor of ObjectStoreData public b2ddba6 <Yuhong Guo> Fix misplace of meta and data in PlasmaClient.java a667402 <Yuhong Guo> Do not evaluate logging strings when logging is not enabled. 5be7b89 <Yuhong Guo> Fix unclosed fd reported by code scan tool 3a917d4 <Yuhong Guo> Fix not used return value in deserialize.cc reported by code scan tool ed56a48 <Yuhong Guo> Fix possible unclosed EventLoop reported by code scanning tool 3fed926 <Yuhong Guo> Add plasma log

guoyuhong commented Feb 15, 2019

View reviewed changes

cpp/src/arrow/util/logging.h Outdated

Copy link

Author

guoyuhong Feb 15, 2019

Choose a reason for hiding this comment

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

There are lint tool conflicts here, so I disable clang-format.

guoyuhong force-pushed the fixPlasma branch from 11158a7 to 950b1a2 Compare February 16, 2019 02:10

pitrou requested changes Feb 18, 2019

View reviewed changes

cpp/src/arrow/python/deserialize.cc Outdated Show resolved Hide resolved

cpp/src/plasma/store.cc Outdated Show resolved Hide resolved

cpp/src/plasma/store.cc Outdated Show resolved Hide resolved

cpp/src/arrow/util/logging.h Outdated Show resolved Hide resolved

pitrou reviewed Feb 18, 2019

View reviewed changes

pitrou approved these changes Feb 18, 2019

View reviewed changes

pitrou force-pushed the fixPlasma branch from 4b93c54 to c3e3527 Compare February 18, 2019 18:29

wesm force-pushed the master branch from 3088183 to 0c6b2d2 Compare February 18, 2019 19:34

kszucs force-pushed the fixPlasma branch from c3e3527 to 470a673 Compare February 18, 2019 22:23

Yuhong Guo and others added 12 commits February 19, 2019 15:17

Add plasma log

3fed926

Fix possible unclosed EventLoop reported by code scanning tool

ed56a48

Fix not used return value in deserialize.cc reported by code scan tool

3a917d4

Fix unclosed fd reported by code scan tool

5be7b89

Do not evaluate logging strings when logging is not enabled.

a667402

Fix misplace of meta and data in PlasmaClient.java

b2ddba6

Make constructor of ObjectStoreData public

434a039

Fix and Lint

79b4af3

Lint

d3eb22f

Address comment

440f097

remove if from ARROW_LOG

b547f2f

Use default argument value to ConvertPyError

634e36a

guoyuhong force-pushed the fixPlasma branch from 470a673 to 634e36a Compare February 19, 2019 07:17

pitrou closed this in 135d481 Feb 19, 2019

guoyuhong deleted the fixPlasma branch February 19, 2019 11:30

guoyuhong mentioned this pull request Feb 22, 2019

Update arrow version to fix plasma bugs ray-project/ray#4127

Merged

asfimport mentioned this pull request Feb 19, 2019

[Plasma] There are bugs reported by code scan tool #21127

Closed

ARROW-4583: [Plasma] Fix some small bugs reported by code scan tool #3656

ARROW-4583: [Plasma] Fix some small bugs reported by code scan tool #3656

Uh oh!

Conversation

guoyuhong commented Feb 15, 2019

Uh oh!

xhochy commented Feb 15, 2019

Uh oh!

guoyuhong commented Feb 15, 2019

Uh oh!

wesm commented Feb 15, 2019

Uh oh!

guoyuhong Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

guoyuhong commented Feb 16, 2019

Uh oh!

guoyuhong commented Feb 18, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

pitrou Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

guoyuhong Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

pitrou commented Feb 18, 2019

Uh oh!

pitrou commented Feb 18, 2019

Uh oh!

wesm commented Feb 19, 2019

Uh oh!

kszucs commented Feb 19, 2019

Uh oh!

pitrou commented Feb 19, 2019

Uh oh!

guoyuhong commented Feb 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants