Closed rixx closed 7 years ago
Naming: I agree it's a bit weird, and maybe it should be renamed asgi_sqlite, but if it still uses IPC and the name is already out there I'm sort of averse to changing it without good reason.
assert
: I am a Lazy Programmer and regularly use assert where I should use something a bit more serious, though in this case I'd say it fits with -O
as you can turn off the extra checking after it's run for a while. Feel free to change things to real exceptions as you want.
The memory dict tests were testing that giant chunk of code you also deleted, so I think that evens out overall. I don't have a problem with the conformance suite being all this runs - it's supposed to be comprehensive!
The prefix is back to being included in db names and semaphore.
The only remaining issue was a string/unicode compatibility issue - I solved it by checking version_info
and using decode
where appropriate in one place, but if you'd prefer, I could also go look for the appropriate six
method …
Yeah, the pattern I've used elsewhere is if isinstance(value, six.binary_type):
for that sort of check, so it would be great if you could switch that over.
Otherwise, I'm very happy with this, and if you think it's ready to merge over I'm happy to do so. If you have spare time and a love for nasty testing environments, you could try benchmarking both the old and new versions, but I can do that later on - and besides, the code simplification alone here makes me much happier.
If you have spare time and a love for nasty testing environments
Neither, really, but for the sake of a clean conscience I'd like to test this for a bit next week. I'll ping you when I'm done.
Haha, alright. I'll try to give it a test drive too.
@rixx did you get a chance to play around with this? I'm going to sit down tomorrow, check, and merge it, just wanted to see if you had.
Nope, I haven't, sorry, been terribly busy. Rebased against master just now.
Just tested it out, looks like there's some thread safety issues, but I'll merge it in and address those in the master branch!
Ooookay, hopefully it's not too bad. 🙈
Nah, nothing worse than what I had to deal with before with shared memory and threading. I'm pushing the commits up to the master branch now as I fix it - almost there!
Will semaphores be needed still, since SQLite can operate in serialized mode (effectively using semaphores itself then)? I too find it odd to move the IPC project to use a database. I think enough improvements can be made to make actual IPC work sufficiently well, and then asgi_sqlite
would be a welcome alternative. If we turn asgi_ipc
into asgi_sqlite
without changing the name, eventually some asgi_real_ipc
will come along.
@thijsvandien I've since moved back from this to a different approach to shared memory as SQLite's locking was failing badly on some platforms, so I guess what you asked for happened already.
asgi_ipc
is in the semaphores, which is kinda weird, naming-wise.asgi_ipc
usesassert
statements all over the place. While I agree thatpython -O
is a mostly theoretical option, I think running a protocol server like this is one of the more probable use cases for-O
. Mind if I rewrite those as explicit exceptions?