search

alex / alchimia

MIT License
102 stars 22 forks source link

use twisted._threads module to address concurrency issues #27

Closed glyph closed 7 years ago

glyph commented 7 years ago

Fixes #25

This pins each TwistedConnection object to a dedicated daemon thread.

glyph commented 7 years ago

NB: twisted._threads is nominally private, but what this means is not "don't use it", just "if you use it, you must make sure you carefully manage your Twisted upgrades in lockstep with your application" and "if it breaks during an upgrade, it's your responsibility to fix" which you should probably be doing anyway. It was my explicit intention that other "close in" ecosystem projects experiment with it to determine if the API is ready to make public, and that's what we're doing here.

glyph commented 7 years ago

26 should probably be merged first, as this includes those revisions.

glyph commented 7 years ago

26 merged.

lvh commented 7 years ago

How do we document the user-facing impact of this? My understanding is that most of the APIs affected are internal, despite not being marked as such through e.g. an underscore prefix.

glyph commented 7 years ago

How do we document the user-facing impact of this? My understanding is that most of the APIs affected are internal, despite not being marked as such through e.g. an underscore prefix.

25 is the main issue in question.

One user-facing impact is that if you use a threadsafety=1 driver, such as sqlite3, with the current release of alchimia, it will just start spewing exceptions at you if you try to use engine.connect rather than engine.execute; so you effectively can't use transactions.

Another user-facing impact is that if you were to try to pipeline SQL to minimize thread-communication overhead on a connection even on a threadsafety=inf driver, i.e. ad = conn.execute(a); bd = conn.execute(b); cd = conn.execute(c); return gatherResults([ad, bd, cd]), the threadpool might arbitrarily re-order the execution of a, b, and c despite being part of the same logical sequence. Of course, getting this to actually happen only happens under load, at scale, etc etc.

Basically, the current approach to putting work into threads is just fundamentally broken, and this fixes it. So the main user-facing message is just "upgrade right now" :)

lvh commented 7 years ago

Could a user ever have passed a thread pool themselves and acquire working software at the end? That seems to be a potentially broken use case.

glyph commented 7 years ago

Could a user ever have passed a thread pool themselves and acquire working software at the end? That seems to be a potentially broken use case.

Not really, no, because the operations passed to the thread pool were not tagged in any way as associated with a connection. The breakage is intentional; hence the leading-zero version number. :). This will probably be released as 0.7.

lvh commented 7 years ago

Once there's a place to track the extraction of the generic bit and you fix the bare except, this looks great and I think @alex should merge it.

glyph commented 7 years ago

@alex gave me commit because he doesn't use this right now and has therefore approximately zero time to work on it :). I'll merge myself once I've addressed those things. Thanks so much for having a look!

glyph commented 7 years ago

All of @lvh's feedback addressed, I will land. Thanks for looking!