Fix Relationship.instances cache. #477
Conversation
marshall-lee
force-pushed
the
fix_relationship_cache
branch
from
8d43e06
to
f993ce2
Compare
|
Hmm, fails only on Ruby 1.9 |
|
@marshall-lee would you be kind enough to rebase and add specs for your changes? |
|
@marhall-lee are you still interested in solving this problem? |
Taken from: randym#477
Taken from: randym#477
|
Ran into this problem today. Luckly I was able to fork the master branch and apply this patch to make axlsx work asynchronously with Sidekiq. |
|
@marshall-lee do you mind applying a rebase on the current master-branch to trigger a fresh travis-ci build?l |
This PR aims to fix several issues with Relationship cache: 1) It's not threadsafe, so I propose to use a TLS variable for this. 2) Memory obtained by cache remains non-freed before the next run of `serialize`. I think it should be freed immediately. 3) Memory should be freed in `ensure` block to prevent memory bloating in case of exception. *There are only two hard things in Computer Science: cache invalidation and naming things.*
marshall-lee
force-pushed
the
fix_relationship_cache
branch
from
f993ce2
to
19b6699
Compare
marshall-lee
force-pushed
the
fix_relationship_cache
branch
from
3a5407f
to
a952a4b
Compare
|
I also rewrote the cache to use the hash table instead of array. And also there's no need to store relationship instances — storing just ids is enough. |
Also cacle only ids, not entire instances.
marshall-lee
force-pushed
the
fix_relationship_cache
branch
from
a952a4b
to
c6dc227
Compare
|
@marshall-lee awesome! thanks for the fast feedback |
|
This PR fixes the thread safety issues we've had with the gem. Hoping this one can be merged soon! |
|
ping @randym |
|
@marshall-lee thank you! :) |
Taken from randym#477
|
@marshall-lee Thanks a lot for working on this! Would you like to recreate this pull request in https://github.com/caxlsx/caxlsx/ ? I think this is an important fix and we should definitively get this merged. |
|
Oh my, I just realized that this has already been done in caxlsx/caxlsx#10 🤦♂️ Sorry for the noise. To minimize this kind of confusion in the future I created and new label “Done in caxlsx” that can be applied to prs / issues that have been included in caxlsx. |
This PR aims to fix several issues with Relationship cache:
serialize. I think it should be freed immediately.ensureblock to prevent memory bloating in case of exception.There are only two hard things in Computer Science: cache invalidation and naming things.