axlsx
axlsx copied to clipboard
randym
Fix Relationship.instances cache.
This PR aims to fix several issues with Relationship cache:
- It's not threadsafe, so I propose to use a TLS variable for this.
- Memory obtained by cache remains non-freed before the next run of
serialize. I think it should be freed immediately. - Memory should be freed in
ensureblock to prevent memory bloating in case of exception.
There are only two hard things in Computer Science: cache invalidation and naming things.
@marshall-lee would you be kind enough to rebase and add specs for your changes?
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. EDIT: Unfortunately this didn't solved my thread problems completely =/
@marshall-lee do you mind applying a rebase on the current master-branch to trigger a fresh travis-ci build?l
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.
@marshall-lee awesome! thanks for the fast feedback @randym anything missing from your side?
This PR fixes the thread safety issues we've had with the gem. Hoping this one can be merged soon!
@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 https://github.com/caxlsx/caxlsx/pull/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.