I wrote a Python script to download files using multiple (source) IP addresses -- kindly suggest any improvements.

import cgi
import os
import posixpath
import Queue
import urllib
import urlparse
import random
import re
import shutil
import time

import requests
import requests_toolbelt

def get_IPs():
"""Returns all available IP addresses in a list."""
# TODO: Windows only. Other options?
out = []
for i in os.popen('ipconfig'):
i = i.strip()
if i.startswith('IP'):
out.append(i.rsplit(' ', 1)[-1])

return out

def get_info(url):
try:
except KeyError:
path = urlparse.urlsplit(url).path
name = posixpath.basename(path)
name = urllib.unquote_plus(name)
return name, size

def worker(url, session, ud, part, size):
# TODO: optimal tries, timeout?
for _ in xrange(2):
try:
open('%s/%04d' % (ud, part), 'wb').write(
session.get(url, timeout=(2, 7), headers={'range': 'bytes=%s-%s' % (
part*chunk, min(size, part*chunk + chunk - 1))}).content)
break
except:
pass
else:
worker(url, sessions_queue.get(), ud, part, size)

sessions_queue.put(session)

def summary(name, size, elapsed):
print (
'--\n'
'Time elapsed: %.2fs\n'
'--' % (name, elapsed, size/elapsed/2**20))

start = time.clock()
name, size = get_info(url)
# random id of length 20
ud = '%0x' % random.getrandbits(80)
os.mkdir(ud)
for i in xrange(size/chunk + (size%chunk != 0)):
t.start()

# characters \/:*?"<>| not allowed in filenames in Windows
name = re.sub(r'[\\/:*?"<>|]', '_', name)
# TODO: check if a file is already present with same name
out = open(name, 'ab')
t.join()

summary(name, size, time.clock() - start)
shutil.rmtree(ud)

def main():
IPs = get_IPs()
print len(IPs), 'IPs available.'
for ip in IPs:
session = requests.Session()
sessions_queue.put(session)

while True:

if __name__ == '__main__':
sessions_queue = Queue.Queue()
KB = 1024
MB = 1024*KB
# TODO: optimal size?
chunk = 100*KB
main()


I am using it with about 100 IP addresses on my Ethernet -- each with about 100 KB/s speed. What'd be optimal configuration? (numbers of threads, chunk size)

• Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – Vogel612 Feb 23 '16 at 12:07

You could rewrite your get_IPs function to be a list comprehension instead:

return [i.rsplit(' ', 1)[-1] for i in map(str.strip, os.popen('ipconfig'))
if i.startswith('IP')]


map will call strip on all of the results from 'ipconfig' and then you can iterate over that, ignoring any values that don't start with "IP".

In worker you're using a loop to retry after timeouts. But you're just using 2 arbitrarily. Use a constant here so it's clear what you're doing, and easy to change later:

You also multiple times open files, but you should always try to use with, known as the context manager. It automatically closes the file even in the event that an error is raised. It's the safest way to open a file.

with open(filepath) as filename:
execute_code_with(filename)
print("Done with filename")


Once you leave that indented block, the file is automatically closed. No need to even call filename.close().

• Thanks for the answer. * I considered a list comprehension for get_IPs function but I thought it was too much of a stretch. * Added a MAX_TRIES variable. (or constant ;) ) * As there are no references to those opened files, I thought Garbage Collector closes them automatically. -- But I just read in docs that we should not rely on it and always close files. – avamsi Feb 23 '16 at 11:18
• Yes GC often will but not reliably, and if you have errors arising they also may not be cleaned up properly. The key value to with is that it ensures files are closed. Also I like list comprehensions, especially when they're abstracted into a small function anyway :P But that's more of a personal opinion. I showed it partially in case you didn't know it was possible. – SuperBiasedMan Feb 23 '16 at 11:22