Server: Dump the store after PUT POST and DELETE requests

[H--o-l]

Localstripe uses the store object to store data between localstripe executions. But I just noticed that some TaxRate attributes are not saved properly. It's the case for a few properties of the TaxRate object.

Reproduction:

• launch localstripe from scratch python3 -m localstripe --from-scratch

• $ curl -sSfg -u sk_test_12345: http://localhost:8420/v1/tax_rates \ -d display_name=VAT -d description='TVA France taux normal' \ -d jurisdiction=FR -d percentage=20.0 -d inclusive=false { "active": true, "created": 1616511744, "description": "TVA France taux normal", "display_name": "VAT", "id": "txr_hG2CLNRGQMy35a", "inclusive": false, "jurisdiction": "FR", "livemode": false, "metadata": {}, "object": "tax_rate", "percentage": 20.0 }

• Kill localstripe

• Relaunch localstripe python3 -m localstripe

• $ curl localhost:8420/v1/tax_rates -u sk_test_12345: { "data": [ { "created": 1616511409, "id": "txr_GDaiJpS2uF5zIj", "livemode": false, "object": "tax_rate" } ], "has_more": false, "object": "list", "total_count": 1, "url": "/v1/tax_rates" }

It's probably the case for other objects too. For me the easiest solution is to always dump the store to the disk just before answering PUT POST and DELETE HTTP requests.

[adrienverge]

Thanks for trying to fix!

If you do it for TaxRate, why other objects wouldn't have the same, like @bravier says? (I don't think it's a good idea, see later.)

super().__init__() is the first instruction, once we're sure the object can be instanciated. It's both logical and intentional, for example some objects need to access their super() id or created value, when setting their properties inside their own __init__().

So there is a bug indeed, but looking at the code it comes from somewhere else (spoiler: see line 50). Can you retry your bug reproduction case, but this time create an object (anyone) after the TaxRate, and before reading the result? I'm curious whether the bug disappears or not.

PS: No period at the end of commit message titles ;)

Indeed, I was able to confirm the same issue with Token object for example. Would you like to split the work between us to fix it in all objects? Don't hesitate to ask ;)

Please try to do it for the Customer, Invoice or PaymentIntent object, and tell me it doesn't crash :)

[bravier]

super().init() is the first instruction, once we're sure the object can be instanciated. It's both logical and intentional, for example some objects need to access their super() id or created value, when setting their properties inside their own init().

Oh okay, it makes sense! Thanks for the explanation.

So there is a bug indeed, but looking at the code it comes from somewhere else (spoiler: see line 50). Can you retry your bug reproduction case, but this time create an object (anyone) after the TaxRate, and before reading the result? I'm curious whether the bug disappears or not.

Indeed the bug disappears in this case. It means the additional properties we set after calling super().__init__() are dumped to disk when we set this second object. But in the case you describe in your reproduction @H--o-l, it's never done. One way to fix it would be to implement signals handling in localstripe so when it gets killed it can dump everything it has in memory to disk.

Please try to do it for the Customer, Invoice or PaymentIntent object, and tell me it doesn't crash :)

Indeed, for these objects it crashes because we need the id for example to instantiate them :+1:

[H--o-l]

Thanks for the PR with great explanation @H--o-l!

:metal:

It looks like this bug is present since the very beginning.

Oo

Don't hesitate to ask ;)

Thanks!

Thanks for trying to fix!

My pleasure!

If you do it for TaxRate, why other objects wouldn't have the same, like @bravier says? (I don't think it's a good idea, see later.)

Wanted your feedback first. It take me some time already to investigate, I didn't wanted to change too much code for nothing (I had doubt on my solution).

super().init() is the first instruction, once we're sure the object can be instanciated. It's both logical and intentional, for example some objects need to access their super() id or created value, when setting their properties inside their own init().

OK, I wasn't sure, make sense.

So there is a bug indeed, but looking at the code it comes from somewhere else (spoiler: see line 50). Can you retry your bug reproduction case, but this time create an object (anyone) after the TaxRate, and before reading the result? I'm curious whether the bug disappears or not.

Indeed the bug disappears in this case. It means the additional properties we set after calling super().init() are dumped to disk when we set this second object. But in the case you describe in your reproduction @H--o-l, it's never done.

That why it was so rare, thanks guys!

One way to fix it would be to implement signals handling in localstripe so when it gets killed it can dump everything it has in memory to disk.

:cry: No other solutions? For example, a dump before the end of each HTTP requests? I think the API shouldn't answer if data are not saved on the disk.

PS: No period at the end of commit message titles ;)

Thanks!

[adrienverge]

Thanks for the great discussion @H--o-l @bravier!

One way to fix it would be to implement signals handling in localstripe so when it gets killed it can dump everything it has in memory to disk.

Oh, very shrewd! I wouldn't have thought of that. It could be a good solution.

For example, a dump before the end of each HTTP requests? I think the API shouldn't answer if data are not saved on the disk

Ah, very smart too!

From my point of view, any of these 2 solutions would be better than the current behavior. @H--o-l would you like to implement one?

PS: @H--o-l handling signals in Python is easy. I'm not saying it's the best solution here (I kind of agree that it's better to write to disk earlier than before quitting), but just for info:

signal.signal(signal.SIGINT, signal_handler)
def signal_handler(signal, frame):
    store.dump_to_disk()

[H--o-l]

OK!

I have implemented a new fix and updated the PR.

I hope @adrienverge @bravier you find it cool!

[bravier]

I have implemented a new fix and updated the PR.

Thanks! This proposal looks really nice. I was concerned that writing to disk often would slow localstripe down but it doesn't look to be the case based on some small experiment I quickly did (it doesn't change much).

PR

./test.sh  0.59s user 0.53s system 83% cpu 1.336 total
./test.sh  0.65s user 0.54s system 84% cpu 1.417 total
./test.sh  0.59s user 0.54s system 83% cpu 1.348 total
./test.sh  0.60s user 0.54s system 83% cpu 1.365 total

Master

./test.sh  0.67s user 0.58s system 85% cpu 1.460 total
./test.sh  0.65s user 0.55s system 86% cpu 1.384 total
./test.sh  0.61s user 0.52s system 86% cpu 1.300 total
./test.sh  0.65s user 0.59s system 86% cpu 1.423 total

[H--o-l]

Nice! It was a really good idea to test it!

[adrienverge]

Nice solution @H--o-l!

And +1 on the idea to measure perf @bravier.

I'm wondering if we could simplify the code by using a middleware? Something like:

@web.middleware
async def save_store_middleware(req, handler):
    try:
        return await handler(req)
    finally:
    (await) store.dump_to_disk()

app = web.Application(middlewares=[error_middleware, auth_middleware,
                                   save_store_middleware])

[H--o-l]

Smart!

I update the new commit with your idea. I allowed myself to add if request.method in ('PUT', 'POST', 'DELETE'):. I think it makes sense. For me, it even helps to understand this code. But don't hesitate to tell me if you disagree.

[H--o-l]

Thanks guys!