malthe / pq

A PostgreSQL job queueing system
376 stars 41 forks source link

Redo timeout handling in get() #55

Closed jobh closed 9 months ago

jobh commented 4 years ago

What is the new behavior?

Suggested fix for #53.

This is a request for comments, not tested yet. The reason is that I don't understand the previous implementation, especially why last_timeout was saved between calls. So, possibly, the new implementation might be misguided.

The new implementation only uses self.timeout as default value for timeout if not specified, never writes to it. last_timeout is removed completely. The timeout (cur_timeout) is recalculated every loop iteration to match the initial deadline.

The following are the intended timeout semantics:

Checklist

Please make sure the following requirements are complete:

jobh commented 4 years ago

Reorder commits: Add (failing) test as first commit, then fix the test failure in second commit.

The state of the PR is now "tested" and merge ready, but still a bit unclear whether there are missed edge cases (regarding the removal of last_timeout).

malthe commented 4 years ago

The reason last_time exists is to reduce latency in the case where we're blocked waiting for an item and there's an item scheduled for processing some time in the future which lies before the timeout.

That is, if we know that in 5 seconds, an item will be ready for processing and the timeout is 10 seconds, then last_time serves to lower the effective timeout to 5 seconds.

That's my interpretation of the semantics around this value. There should be a test case in there which demonstrates this behavior.

stas commented 4 years ago

I second @malthe and find it hard to understand why last_timeout is the issue here. Let's discuss the issue #53 first and jump to the coding once we're all on the same page please.