Store Promise<Response> instead of Response for HTTP API transactions

[kegsay]

This fixes a race whereby:

• User hits an endpoint.
• No cached transaction so executes main code.
• User hits same endpoint.
• No cache transaction so executes main code.
• Main code finishes executing and caches response and returns.
• Main code finishes executing and caches response and returns.

This race is common in the wild when Synapse is struggling under load.
This commit fixes the race by:

• User hits an endpoint.
• Caches the promise to execute the main code and executes main code.
• User hits same endpoint.
• Yields on the same promise as the first request.
• Main code finishes executing and returns, unblocking both requests.

Now with bonus sytests!

[erikjohnston]

I wondering if a nicer API would be something like:

self.transactions.fetch_or_execute(
    self.handler.do_foo, txn_id,
    arg1, arg2, arg3=arg3
)

where fetch_or_execute would use the first arg as a txn_id, and if that wasn't in the cache call the given function with the arguments. This has the advantage you don't explicitly need to remember to both check and store.

[erikjohnston]

Also, would be totally awesome if HttpTransactionCache had some python tests. Maybe also move it to synapse.utils?

[kegsay]

fetch_or_execute doesn't appear to pass the request object which is used to select the key in the cache. Was this intentional?

[kegsay]

Also, I don't mind moving it to util, but that feels like a downgrade in specificity, since only the REST Servlets make use of this class, and the aforementioned reliance on a request object.

[kegsay]

I'm guessing you're proposing I make it more generic (so txn_id is just the key in the cache)? Do we plan on using the generic form elsewhere?

[erikjohnston]

fetch_or_execute doesn't appear to pass the request object which is used to select the key in the cache. Was this intentional?

Nah, I just made it up.

Also, I don't mind moving it to util, but that feels like a downgrade in specificity, since only the REST Servlets make use of this class, and the aforementioned reliance on a request object.

At the very least it should be moved up, but generally I quite helpers like this to live a bit separately, rather than being dumped alongside the rest servlets themselves

I'm guessing you're proposing I make it more generic (so txn_id is just the key in the cache)? Do we plan on using the generic form elsewhere?

Well, it is currently implemented in a generic fashion. I'm happy for the arg name to be txn_id or key or whatever

[kegsay]

At the very least it should be moved up, but generally I quite helpers like this to live a bit separately, rather than being dumped alongside the rest servlets themselves

synapse/rest/client/transactions.py perhaps?

Well, it is currently implemented in a generic fashion. I'm happy for the arg name to be txn_id or key or whatever

Do you want the implementation to be generic or are you happy with it in its current form (accepting request objects which are then processed for their access_token for use as a key)?

[erikjohnston]

synapse/rest/client/transactions.py perhaps?

That's fine i suppose

Do you want the implementation to be generic or are you happy with it in its current form (accepting request objects which are then processed for their access_token for use as a key)?

Oh, I misread. Yeah, ok, I guess the generation of the key is non-trivial. Though I'd still be tempted to move the _get_key onto the v1 rest base class, as that would make the transaction class a nice self-contained and easily testable class, rather than having it know about request objects.

[kegsay]

SGTM

[kegsay]

Hmmm. The old implementation was using transaction IDs as a way to prune the cache, but it means that you couldn't have multiple in-flight requests at the same time and get idempotency, which feels bad. I've removed that code in my fix, but now the cache will grow unbounded.

How do you propose I clear the cache? Periodic interval? 10 minutes? The generic form now just takes a key, so I can't be more intelligent like base it off the given user (access_token, which is now concatenated in the key).

[erikjohnston]

I think you need a .observe() on the end

[kegsay]

Done.

[erikjohnston]

Ditto a .observe() here too

[kegsay]

Done.

[erikjohnston]

Ah, I'd move this .observe() up into the actual cache to make things neater:

def on_PUT(self, request, txn_id):
    return self.txns.fetch_or_execute_request(
        request, self.on_POST, request
    )

[kegsay]

Done.

[erikjohnston]

How do you propose I clear the cache? Periodic interval? 10 minutes? The generic form now just takes a key, so I can't be more intelligent like base it off the given user (access_token, which is now concatenated in the key).

For now, I'd probably expire after 30mins (10 is probably a bit on the low side). Ideally I'd guess we'd probably batch persist these txn_ids to the db so they survive restarts, and then purge that table after a few hours/days.

[erikjohnston]

(Also, a python test case for the HttpTransactionCache class would be awesome)

[kegsay]

(Also, a python test case for the HttpTransactionCache class would be awesome)

Done.

For cleaning entries, I'm just periodically checking every 30 minutes, and timestamping when functions were invoked (which means the actual time in the cache is between 30~60 minutes). This feels simpler and less wasteful compared to registering timeouts for each entry in the cache, which has comparatively more function call overhead.

[kegsay]

@erikjohnston PTAL

Also, are the Dendron tests just flakey or should I be worried? Looking at the previous builds on http://matrix.org/jenkins/job/SynapseSytestDendronCommit/ makes me think flakey, but I don't know.

[erikjohnston]

Also, are the Dendron tests just flakey or should I be worried? Looking at the previous builds on http://matrix.org/jenkins/job/SynapseSytestDendronCommit/ makes me think flakey, but I don't know.

Yes :(

[erikjohnston]

LGTM