2
\$\begingroup\$

I have an api endpoint that, when called, initiates the generation of a report. This is time consuming process and response could come pretty late. So, we came up with caching architecture and once finished the response is saved to redis.

However, on the frontend side we dicided to make a request once in a while, until it is ready. To face the issue of overlapping queries when one report is initialized several times in a row, I decided to make a temporary file, that exists only when the report is being generated, and the api response is "This report is already being generated". Once the generation is finished, file is deleted and response is already cached.

This is mockup of what I am doing

def post(self):
   serialized_data = self.serializer().deserialize({
            'task': json.loads(request.values.get('task')),
            'media_company': json.loads(request.values.get('mediaCompany'))
    })

   if not os.path.exists('/root/progress_data'):
        os.mkdir('/root/progress_data')

    self.build_range(serialized_data)

    serialized_data['media_company']['date_from'] = \
                                        self.date_from.strftime("%Y-%m-%d")
    serialized_data['media_company']['date_to'] = \
                                        self.date_to.strftime("%Y-%m-%d")
    progress_hash = hashlib.sha256(str(serialized_data).encode()).hexdigest()

    if self.progress_check(progress_hash):
        return json_response({
            "success": False,
            'result': "This report is already being generated",
        })

    file = open(f'/root/progress_data/{progress_hash}', 'w+')
    file.close()
    try:
        report = self.generate_report(serialized_data)
    except:
        os.remove(f'/root/progress_data/{progress_hash}')
        return json_response({
            "success": False,
            'result': "Error while generating report",
        })
    os.remove(f'/root/progress_data/{progress_hash}')
    return json_response({
        "success": True,
        'data': report,
    })

I think that this is not really production-ready solution, but I could not come up with something better.

Also, there are some holes where the file is not being deleted in all cases.

Could you show me potential holes and may be another way of checking the progress

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Is this the actual code, or a mockup of the code? On code review we review working code that you have written. If this is not the actual code then the question is off-topic and may be closed. \$\endgroup\$
    – pacmaninbw
    Jul 6 '20 at 15:07
  • \$\begingroup\$ @Anonymous writes I am not sure your question really belong here. We can comment on code that is working as expected but could be perfectible. Your code has holes as you put it, so it cannot be considered a finished piece of work. \$\endgroup\$
    – Reinderien
    Jul 6 '20 at 16:26
  • \$\begingroup\$ You need to show the code for generate_report. \$\endgroup\$
    – Reinderien
    Jul 6 '20 at 16:47
2
\$\begingroup\$

There is nothing I can test here, so I will only make a couple superficial remarks.

You do not offer a lot of insight into your application. If you say you have a problem with overlapping queries, then you should address the root of the problem. I have the impression that you simply devised a workaround. It may do the job but at the expense of performance or reliability. It could also create more problems.

Repetition

What is immediately apparent is that you have a hardcoded path repeated multiple times across your code. At the top of your code you should define a variable for '/root/progress_data'. Then if you decide to change the file name or path you will have only one line to change.

But Python has a library for temp files: https://docs.python.org/3/library/tempfile.html. This is what you should be using.

The date format "%Y-%m-%d" could be made a variable too.

Security

Does your program really need to run as root ?

Maximize your try/catch/finally blocks

You repeat this twice:

os.remove(f'/root/progress_data/{progress_hash}')

To ensure that the file gets removed at the end of the process you could place this line of code in a finally block.

Same goes for the return value:

return json_response({
    "success": False,
    'result': "Error while generating report",
})

return json_response({
    "success": True,
    'data': report,
})

Keep only one return but use variables for the response depending on the outcome of the execution.

Thus your code will be shorter, more straightforward and easier to comprehend. But I still think the idea is wrong.

I would probably use a database instead of temp files. It can be a small self-contained DB file like SQLite. You need a simple table with details of the job, some kind of unique identifier and a column to update the status of the job. Then your API can query the table and say "ready" when the column has been to updated to the finished status.

This looks clumsy to me. I would ditch the idea and go back to the drawing board. If you are stuck my suggestion would be to post on Stack Overflow for guidance but add more context to your question, explain how the API is used, what your constraints are.

\$\endgroup\$
1
  • \$\begingroup\$ Does your program really need to run as root? - a million times this. \$\endgroup\$
    – Reinderien
    Jul 6 '20 at 16:36
1
\$\begingroup\$

Round-trips for serialization

You start with this:

request.values.get('task')

Then loads it to (presumably) a dictionary; wrap it in an outer dictionary; and then immediately deserialize it. This seems a little awkward. If task is a serialized representation that needs special deserialization logic, then can you factor out a section of deserialize and only call that, rather than these intermediate steps?

Do not run as root

This is stated by @Anonymous and bears repeating. Do not run as root. This is dangerous and bad. You should be making a service user with limited permissions and running this script as that user. In addition, consider replacing this:

/root/progress_data

with a subdirectory of var whose permissions are restricted to the service user.

Dict updates

serialized_data['media_company']['date_from'] = \
                                    self.date_from.strftime("%Y-%m-%d")
serialized_data['media_company']['date_to'] = \
                                    self.date_to.strftime("%Y-%m-%d")

can be

serialized_data['media_company'].update({
    'date_from': self.date_from.strftime("%Y-%m-%d"),
    'date_to': self.date_to.strftime("%Y-%m-%d"),
})

Choose a string style

i.e. single or double quotes, rather than

'result': "This report is already being generated"

Creation-closure

This block:

file = open(f'/root/progress_data/{progress_hash}', 'w+')
file.close()
try:
    report = self.generate_report(serialized_data)
except:
    os.remove(f'/root/progress_data/{progress_hash}')
    return json_response({
        "success": False,
        'result': "Error while generating report",
    })
os.remove(f'/root/progress_data/{progress_hash}')

has a few issues:

  • Rather than an explicit close, put the open in a with
  • As @Anonymous says, move your remove to a finally and de-duplicate it
  • Form the progress file path using pathlib.Path
  • Store the path to a temporary variable rather than re-calculating it three times.
  • Do not base the name of the file on a hash, and do not open it yourself; allow tempfile to name and open it for you. If necessary, you can control the temporary file's directory with the dir argument.
\$\endgroup\$
1
\$\begingroup\$

possible race condition

This code seem to have a race condition:

if self.progress_check(progress_hash):
    return json_response({
        "success": False,
        'result': "This report is already being generated",
    })

file = open(f'/root/progress_data/{progress_hash}', 'w+')

The question doesn't say what server is being used, but it presumably uses threads, processes or async techniques. After a first thread or process executes the if statement but before the file gets opened, other threads or processes could pass the if statement. This can result in multiple computations, multiple files, trying to delete a file multiple times, etc. I think some kind of lock or semaphore may be needed.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.