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
AblyInstanceStore
concurrency
#321
Conversation
The existing reserveClientHandle() was assumed to always be called from the same thread, with no logical link to the createRest() and createRealtime() that were being called after it. I'm honestly not sure if this was a bug that was being experienced right now, but the assumptions that the existing AblyMethodCallHandler implementation were making were likely to become an issue for us later on as the codebase grows, so I wanted to get this fixed now.
I considered have a discrete lock for paginated results but kept on circling back to the implications of the work done by the reset() method. So, on balance, I can't envisage a scenario where any of these methods take such a long time with the lock obtained that this change would have a measurable impact on performance.
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.
Looks good, I've only spotted two things 😉
android/src/main/java/io/ably/flutter/plugin/AblyInstanceStore.java
Outdated
Show resolved
Hide resolved
Converting this PR back to Draft state while I work on @KacperKluka's review comments. |
I was reinventing the wheel.
…the Ably instance store. I had overlooked the requirement to actually set the handle as used when it had been used, being the whole point as to why this class was there!
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.
The solution looks fine to me 🙂
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.
LGTM 👍
Fixes #308.