Raise ResultNotFound when Job.result() finds no job and no result

[iamlikeme]

Job.result() assumed that a result will eventually show up in Redis. This assumption does not hold for jobs which are not in the queue and jobs which do not keep result. In such cases Job.result() would hang forever (or until the optional timeout is reached).

This PR changes Job.result() so that it raises a new exception ResultNotFound, rather than waiting forever, if result is not found and the job is not in the queue.

[codecov]

Codecov Report

Merging #364 (96b094d) into main (1495be6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 96b094d differs from pull request most recent head 019450a. Consider uploading reports for the commit 019450a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
+ Coverage   98.80%   98.81%   +0.01%     
==========================================
  Files          11        9       -2     
  Lines        1007      932      -75     
  Branches      185      172      -13     
==========================================
- Hits          995      921      -74     
  Misses          6        6              
+ Partials        6        5       -1     

Impacted Files	Coverage Δ
arq/jobs.py	98.14% <100.00%> (+0.14%)	⬆️
arq/__init__.py
arq/cron.py
arq/worker.py	99.56% <0.00%> (+0.22%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1495be6...019450a. Read the comment docs.

[samuelcolvin]

Looks like a good idea to me, just a few things to change to change.

Also can you add some info about this to result docstring.

[samuelcolvin]

I'll try to make a new release tomorrow, would be great if we could include this.

[iamlikeme]

I updated docstring and error message. If you want to change wording feel free to push directly to this branch.

[iamlikeme]

The new ResultNotFound exception may now occur in Job.abort(). When it does, we don't know whether the job was cancelled or not (e.g. the job might have been cancelled, but if it does not keep result we wouldn't know). There are several options for what we can do with it:

• not handle it, i.e. let Job.abort() raise ResultNotFound
• catch and return False (i.e. job wasn't cancelled)
• catch and return True (i.e. job was cancelled)
• catch and return something else, e.g. None - this would be a breaking change

The first two options seem most sensible to me and I chose to catch and return False. Let me know if you'd rather have it changed.

[samuelcolvin]

Great, thanks so much.

The first two options seem most sensible to me and I chose to catch and return False

Agreed, I think returning False is most correct since it's matches the docstring: True if the job aborted properly, False otherwise

[shahakL]

Hi @iamlikeme! Quick question about this PR, I noticed this part in Job.result():

async with self._redis.pipeline(transaction=True):
    v = await self._redis.get(result_key_prefix + self.job_id)
    s = await self._redis.zscore(self._queue_name, self.job_id)

A transaction is created but not used for the two queries; is that on purpose? Couldn't it create a possible false ResultNotFound if the job is done and removed from the queue after the first line but before the second?

[iamlikeme]

Hi @shahakL! You are completely right. What a great catch, thank you! The fix is in #374.

[shahakL]

@iamlikeme Thank you for your contributions!