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 potential race condition in 'download_checkmd5.py' #715

Merged
merged 1 commit into from
Feb 4, 2015

Conversation

czalidis
Copy link
Contributor

@czalidis czalidis commented Feb 4, 2015

When trying to download multiple files in the same directory, using catkin_download_test_data, there is a race condition (if make has been called with -jN, where N>1) because 2 or more threads are trying to create the same directory.

I have created a simple package to reproduce that issue. The only thing that package does, is to declare 2 calls to catkin_download_test_data in the CMakeLists.txt, where the DESTINATION is the same directory.

The error I get is the following:

#### Running command: "make tests -j8 -l8" in "/home/chris/catkin_ws/build"
####
Scanning dependencies of target test_download_data_32e.pcap
Scanning dependencies of target test_download_data_test.pgm
[100%] [100%] Generating /home/chris/catkin_ws/devel/share/test_download_data/tests/32e.pcap
Generating /home/chris/catkin_ws/devel/share/test_download_data/tests/test.pgm
Downloading http://download.ros.org/data/maps/test.pgm to /home/chris/catkin_ws/devel/share/test_download_data/tests/test.pgm...Traceback (most recent call last):
  File "/home/chris/catkin_ws/src/catkin/cmake/test/download_checkmd5.py", line 177, in <module>
    sys.exit(main())
  File "/home/chris/catkin_ws/src/catkin/cmake/test/download_checkmd5.py", line 151, in main
    download_md5(uri, args.dest)
  File "/home/chris/catkin_ws/src/catkin/cmake/test/download_checkmd5.py", line 91, in download_md5
    os.makedirs(dirname)
  File "/usr/lib/python2.7/os.py", line 157, in makedirs
    mkdir(name, mode)
OSError: [Errno 17] File exists: '/home/chris/catkin_ws/devel/share/test_download_data/tests'
make[3]: *** [/home/chris/catkin_ws/devel/share/test_download_data/tests/32e.pcap] Error 1
make[2]: *** [test_download_data/CMakeFiles/test_download_data_32e.pcap.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....
 done.
Checking md5sum on /home/chris/catkin_ws/devel/share/test_download_data/tests/test.pgm
[100%] Built target test_download_data_test.pgm
make[1]: *** [CMakeFiles/tests.dir/rule] Error 2
make: *** [tests] Error 2

You can reproduce the same error, cloning that package and running catkin_make tests.

This pull request is supposed to resolve that issue.

if len(dirname):
try:
os.makedirs(dirname)
except OSError, e:
Copy link
Member

Choose a reason for hiding this comment

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

Please use Python 3 compatible syntax:

except OSError as e:

@czalidis czalidis force-pushed the fix-download-checkmd5 branch from 07a0ee4 to f73a486 Compare February 4, 2015 01:29
@wjwwood
Copy link
Member

wjwwood commented Feb 4, 2015

Other than the comments that looks reasonable, +1. Thanks!

@czalidis czalidis force-pushed the fix-download-checkmd5 branch from f73a486 to 74c1e65 Compare February 4, 2015 01:37
try:
os.makedirs(dirname)
except OSError as e:
import errno
Copy link
Member

Choose a reason for hiding this comment

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

Please move the import to the top of the file (between __future__ and os).

@dirk-thomas
Copy link
Member

Also please comment on the ticket when committing changes. Otherwise we don't get a notification.

@czalidis czalidis force-pushed the fix-download-checkmd5 branch from 74c1e65 to a9ad503 Compare February 4, 2015 19:33
@czalidis
Copy link
Contributor Author

czalidis commented Feb 4, 2015

Ok, sorry for the inconvenience. I have looked into the comments and modified my commit accordingly.

@dirk-thomas
Copy link
Member

Np. Thank you for the patch.

dirk-thomas added a commit that referenced this pull request Feb 4, 2015
Fix potential race condition in 'download_checkmd5.py'
@dirk-thomas dirk-thomas merged commit 6d9e3eb into ros:indigo-devel Feb 4, 2015
cwecht pushed a commit to cwecht/catkin that referenced this pull request Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants