Skip to content
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

Fix freeze on pad deletion #3562

Merged
merged 2 commits into from
Apr 26, 2020
Merged

Fix freeze on pad deletion #3562

merged 2 commits into from
Apr 26, 2020

Conversation

Chocobozzz
Copy link
Contributor

Fix freeze on pad deletion (if it has many revisions)

Needs ether/ueberDB#118 first (because of https://github.com/Pita/ueberDB/pull/118/files#diff-0a774d4a9cc6e6d690c76811449906c1R214)

@muxator
Copy link
Contributor

muxator commented Mar 7, 2019

Now that we implemented #3540 this patch should be adapted to async/await style.

@raybellis
Copy link
Contributor

I think this needs to be tested with the async version to see whether it still even causes any freezes.

The new code doesn't explicitly wait for any of the db.remove() operations to complete before returning, they're all just sent to the backend database engine asynchronously.

@Chocobozzz
Copy link
Contributor Author

@raybellis
Copy link
Contributor

Sorry, I was talking about the new async code in the develop branch, not the new code in your patch.

@muxator muxator added async-migration Migration from callback-style programming to async functions performance labels Jun 23, 2019
@muxator
Copy link
Contributor

muxator commented Nov 26, 2019

@Chocobozzz, any chance you are able to forward port this fix to the current develop? It would be nice if it gets included in 1.8.0.

@Chocobozzz
Copy link
Contributor Author

Sorry for the late. Can I add bluebird as dependency to use the map function?

@Chocobozzz
Copy link
Contributor Author

ping @muxator

@muxator
Copy link
Contributor

muxator commented Dec 19, 2019

Hi, @Chocobozzz, sorry for being late.

I just tested installing it and bluebird has no dependencies and is not too big (~700 KB).
May there any way of accomplishing what you need just using the standard library?
If not, please go on adding it.

~/test$ npm install bluebird
+ bluebird@3.7.2
added 1 package from 1 contributor in 0.247s
~/test/node_modules$ du -sch bluebird/
712K    bluebird/
712K    total

@raybellis
Copy link
Contributor

IMHO, adding Bluebird is excessive just to get access to a Promise .map function. In the new async code there are plenty of instances where I've used trivial "native" code to implement similar functionality.

@Chocobozzz
Copy link
Contributor Author

IMHO, adding Bluebird is excessive just to get access to a Promise .map function. In the new async code there are plenty of instances where I've used trivial "native" code to implement similar functionality.

Where do you use a map with a concurrency limit?

@raybellis
Copy link
Contributor

Fair enough, although you didn't call that out as a specific requirement in your suggestion.

@muxator
Copy link
Contributor

muxator commented Dec 19, 2019

Before merging this PR, we have to remember that we depend on a new release of ueberdb (that incorporates ether/ueberDB#118), or we have to go on with #3675.

@Chocobozzz Chocobozzz force-pushed the pad-deletion branch 2 times, most recently from ac29074 to 02110da Compare December 19, 2019 16:28
@Chocobozzz
Copy link
Contributor Author

Chocobozzz commented Dec 19, 2019

I created a custom function instead of adding bluebird and updated the PR

@JohnMcLear
Copy link
Member

FWIW the UeberDB patch got merged ages ago and it's released on npm so should be good from UeberDB side. cc @muxator

@muxator
Copy link
Contributor

muxator commented Mar 24, 2020

This will be pulled in as soon as #3734 is integrated.

@muxator
Copy link
Contributor

muxator commented Apr 1, 2020

We can go on with this PR, since ueberDB was updated via #3734.

But I see there are conflicts.

If the conflicts are trivial I can take care of it. If they end up being more that pure dumb text manipulation I will need support from @Chocobozzz.

@muxator
Copy link
Contributor

muxator commented Apr 1, 2020

@Chocobozzz: before merging, I would like - please - that you insert a short note explaining the purpose of src/node/utils/promises.js for the posterity.

I think I got the reason for the conflicts: your develop branch still points to 4944945. There are no particular conflicts otherwise, is a well contained change. I can take care of that if updating your repo is complicated.

Just write me a bit of documentation. 😄

@Chocobozzz
Copy link
Contributor Author

Done 👍

@muxator muxator force-pushed the pad-deletion branch 2 times, most recently from 27aa97d to 2d5f5a0 Compare April 26, 2020 02:12
@muxator muxator added this to the 1.8.3 milestone Apr 26, 2020
@muxator
Copy link
Contributor

muxator commented Apr 26, 2020

This was long overdue.

Merged & queued for 1.8.3, thanks.

@muxator muxator merged commit 6cb78e5 into ether:develop Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async-migration Migration from callback-style programming to async functions performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants