-
-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add possibility to use locking with DynamoDB sessions #1676
base: master
Are you sure you want to change the base?
Conversation
ef20bb1
to
524c26d
Compare
524c26d
to
781163e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly,
Lock is acquired when reading the session
and released when writing the session.
This sound really fragile to me.
- What if session is written several time?
- What if the session is not written (just read and close)
- What if the app crashes before writing the session? Session will stay locked forever?
The last question make me think, this is not the right way, and we should use symfony/lock in a decorator instead
$options['consistent_read'] = $options['consistent_read'] ?? true; | ||
$options['locking'] = (bool) ($options['locking'] ?? false); | ||
$options['max_lock_wait_time'] = (float) ($options['max_lock_wait_time'] ?? 10.0); | ||
$options['min_lock_retry_microtime'] = (int) ($options['min_lock_retry_microtime'] ?? 10000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure such thing deserve parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand DynamoDB usually is billed per-request, so giving a possibility to fine-tune these according to your use case may be useful in terms of balancing the functionality & cost-savings. For example, if your sessions usually are locking for several seconds, you likely don't want to be polling every 10-50ms (which again is the default from official sdk).
Also, these settings are just copied from aws-sdk to make a migration to async-aws as small change as possible.
Thanks for the quick review @jderusse!
The logic is mostly copied from the official aws-sdk, so if you think it's fragile, then the official sdk is fragile?
On aws/src/Integration/Aws/DynamoDbSession/src/SessionHandler.php Lines 115 to 116 in bc92cce
Right now, correct, the same as with the official aws-sdk. But of course we could store a timestamp in
While in general I like the idea, I'm not sure how great that would be in terms of performance, integrity & reliability (having separate storage/requests/connections for locks etc) + again it would defeat the purpose of this PR which is to make it easier to migrate from If we were to use |
DynamoDB session handler in this library not supporting session locking makes it really difficult to migrate applications using the official AWS SDK with locking (or default file handler with locking) to
async-aws/dynamo-db-session
.Unless there are some valid arguments not to support locking, I'd propose let's add it.