Handle opt.destroy() being a promise #16

alubbe · 2019-01-21T12:56:35Z

knex.js passes a function as opt.destroy that creates a promise, so I believe the .destroy() should await completion. My PR works the same as previously if opt.destroy does not create a promise.

This was found via bluebird highlighting this promise creation.

alubbe · 2019-01-21T13:21:16Z

(the node 8 test failure seems unrelated)

koskimas · 2019-01-21T13:44:49Z

Did you handle all the cases where _destroy is used? You also need to add a test for each of those cases when the destroyer is async.

koskimas · 2019-01-21T13:45:40Z

Also now what happens with synchronous errors (a log message) and what happens with async errors (leaked error) is different. Those should probably be handle the same way.

alubbe · 2019-01-21T13:55:52Z

Did you handle all the cases where _destroy is used? You also need to add a test for each of those cases when the destroyer is async.

The other uses seem to be triggered

by .check(), which is run in an interval, so, as far as I can tell, it wasn't checked for whether the previous run failed, is still ongoing or was completed
by ._tryAcquireOrCreate(), whose completion status is never checked

So I left those as they are - I think it's fine actually. If not, a larger rethink is probably necessary. What do you think?

Also now what happens with synchronous errors (a log message) and what happens with async errors (leaked error) is different. Those should probably be handle the same way.

I don't think that's the case in line 158. If a sync error happens, it's caught by the .map, which passes it to the promise.all, which passes it to the .then. Similarly, if there is an async error, it gets re-thrown by promise.all, which passes it to the .then.

koskimas · 2019-01-21T13:59:08Z

Errors are not passed from Promies.all to then. They move forward to the next catch block or if there is non, they leak as unhandled exceptions. Same with the check calls. The async errors are not caught and they leak as unhandled exceptions.

Write the tests and you'll see. This won't get merged until you test all the cases with both failing handlers and working handlers.

alubbe · 2019-01-21T14:45:35Z

Alright, have a look and let me know what you think

alubbe · 2019-01-25T14:40:57Z

Any feedback?

koskimas · 2019-01-26T14:44:09Z

Sorry, I'm on vacation for three weeks without my laptop. I'll get back to you after that.

alubbe · 2019-03-12T12:30:58Z

hey @koskimas I hope you had a great vacation - any chance you could take a look at this PR again?

koskimas · 2019-03-13T17:09:34Z

I did, but now I'm swamped with work and with life in general. I just don't have time for this project. You can try any of the other contributors.

wubzz · 2019-03-14T05:31:36Z

lib/Pool.js

+ // There's nothing we can do here but log the error.
+ retVal.catch(this._logError.bind(this));
+ }
+ return retVal;


This part I don't understand. Assuming destroyer is a promise now returning the value like this makes the try-catch pretty redundant since it's not 'awaited' for.

Isn't this equivalent to the current catch behaviour? While we don't await for it, I guess we still want to log it to not hide it from the user (or at least that's why I assume we're logging the error for the sync destroyer)

What I mean is calling try/catch on a promise without async/await is redudant. Error is caught in .catch handler instead. Can remove try catch block completely.

it's meant to catch caused by this.destroyer when it's synchronous and not a promise - since that's the old behaviour. Should I make a comment into the catch block to make that clearer?

Since destroy is returning promise now shouldn't it convert that catched error to caller as return Promise.reject(err)? Also I'm not sure at all if _destroy() should catch all error internally anymore... for this use case to wait that pool is completely torn down it probably doesn't matter.

I'm happy to make that change - I have no insight into why the error was silenced in the sync case. could you as the maintainers decide what you'd like here?

For now, I will keep the current behaviour - i.e., hide errors & always resolve
Let me know if you want to change that

wubzz · 2019-03-14T05:36:22Z

lib/Pool.js

@@ -155,7 +155,9 @@ class Pool {
 })
 .then(() => {
 // Now we can destroy all the freed resources.
- this.free.forEach(free => this._destroy(free.resource));
+ return Promise.all(this.free.map(free => this._destroy(free.resource)));


I understand the point of this, but I think compared to how Tarn used to handle this, it is now potentially flawed since:

There's no timeout set for destroy, it could hang forever in worst case

It doesn't clear current state after potential error occurs which it did before

This could potentially leave the tarn instance in a state where it has 'tried' to destroy all active resources - but failed - and there's no way to create new resources because it is filled with faulty resources.

I could be thinking too far into it and may be mistaken, but this is my interpretation of this change..

Regarding 1., how about we use https://github.com/sindresorhus/p-timeout ?
Regarding 2., sorry I don't understand - how/where does it currently clear state that it doesn't anymore with these changes?

Also Promise.all rejects right away if one of the destroy commands fail. So there might be 200 destroys running at the same time, first one rejects and Promise.all rejects right away, even that there are 199 promises still unresolved. (see for example this https://stackoverflow.com/questions/31424561/wait-until-all-es6-promises-complete-even-rejected-promises)

EDIT: sorry, I just expanded some code above this. Looks like reflect stuff is already implemented there 👍 so yeah this comment was just wrong and can be ignored.

EDIT2: ... except look like actually reflect should be used here too, because otherwise this.free = []; is set too early and we return too early...

EDIT3: Since _destroy() seems to always catch all errors internally, I suppose that currently _destroy actually never rejects... anyhow it would be safer to use reflect anyways in case if functionality of _destroy is changed at some point.

Regarding 1., how about we use https://github.com/sindresorhus/p-timeout ?

Since tarn.js actually does have also own timeout for acquire and create already, it would make sense to have it for destroy. No need to include any additional deps for it.

Regarding 2., sorry I don't understand - how/where does it currently clear state that it doesn't anymore with these changes?

Since _destroy never rejects, I think that functionality is still the same. Anyways reflect should be added there just to be on safe side for future, so that rejected promises will not cause Promise.all to reject too early.

Added that as well as an destroyTimeoutMillis option

alubbe · 2019-03-18T09:55:00Z

Made some more changes - looking forward to your input!

elhigu · 2019-03-19T19:27:08Z

README.md

@@ -55,6 +55,10 @@ const pool = new Pool({
 // if a resource cannot be acquired
 createTimeoutMillis: 30000,

+ // destroy operations are awaited for at most this many milliseconds
+ // new resources will be created after this timeout
+ destroyTimeoutMillis: 200,


This sounds way too small timeout, better to wait few seconds to be sure. I wouldn't think that waiting for example 5 seconds in case of weird problems here would be a problem. Better to wait a bit longer than timeout too soon, before destroy is ready.

should be changed to be 5000 here too to be consistent

elhigu

Looks really good, I didn't find anything to comment of implementation, except that destroy default timeout could be longer. I think it is better to have to wait up couple of seconds in case of hanging destroy instead of accidentally calling timeout and returning when pool has not yet released all the resources.

Also maybe I missed something, but I didn't see that any test was making sure that destroy timeout prevents destroy call from hanging. After those changes I think this is good 👍

alubbe · 2019-03-20T16:05:24Z

I increased the timeout and added that test - let me know what you think

alubbe · 2019-03-28T09:06:19Z

@elhigu could you take another look if you think it's fine now?

elhigu · 2019-03-28T09:32:19Z

src/Pool.ts

+ });
+
+ // In case of an error there's nothing we can do here but log it.
+ return pendingDestroy.promise.catch(this._logError.bind(this));


Also this would be more clear without bind like this:

return pendingDestroy.promise.catch(err => this._logError(err));

elhigu · 2019-03-28T09:33:53Z

@alubbe sorry for delay, I still managed to find two minor issues (in readme and using .bind() when it is not necessary). If you can fix them I'll merge this ASAP. Thanks for your patience!

alubbe · 2019-03-31T08:38:01Z

done! :)

elhigu · 2019-03-31T21:00:35Z

Thank you very much! I'm not entirely sure if this is a breaking change or not... I would say its not and just bump minor version. Any opinions?

alubbe · 2019-04-01T12:47:52Z

I agree - the docs expect a sync destroyer, for which the behaviour is unchanged. And for consumers like knex, who ended up returning a promise, this seemed to be working, so I it feels unlikely this feature could breakt it

Handle opt.destroy being a promise

b63ac10

alubbe mentioned this pull request Jan 21, 2019

Explicitly silence bluebird warnings #17

Merged

Add tests

e547c4b

koskimas mentioned this pull request Feb 13, 2019

Destroy should expect a Promise #15

Closed

wubzz reviewed Mar 14, 2019

View reviewed changes

alubbe added 2 commits March 18, 2019 10:03

Add a comment explaining error handling of this.destroyer()

8d23bcf

Add destroy timeout

78903bd

alubbe force-pushed the opt-destroy-promise branch from cedf7ad to 78903bd Compare March 18, 2019 09:50

elhigu reviewed Mar 19, 2019

View reviewed changes

elhigu requested changes Mar 19, 2019

View reviewed changes

Increase destroy timeout to 5 seconds and add test

0c79f9e

elhigu reviewed Mar 28, 2019

View reviewed changes

Update README and avoid .bind()

2d2c460

alubbe force-pushed the opt-destroy-promise branch from 5ab8a21 to 2d2c460 Compare March 31, 2019 08:37

elhigu approved these changes Mar 31, 2019

View reviewed changes

elhigu merged commit 717cd49 into Vincit:master Mar 31, 2019

alubbe deleted the opt-destroy-promise branch April 1, 2019 12:46

alubbe restored the opt-destroy-promise branch April 1, 2019 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle opt.destroy() being a promise #16

Handle opt.destroy() being a promise #16

alubbe commented Jan 21, 2019

alubbe commented Jan 21, 2019

koskimas commented Jan 21, 2019

koskimas commented Jan 21, 2019

alubbe commented Jan 21, 2019 •

edited

Loading

koskimas commented Jan 21, 2019

alubbe commented Jan 21, 2019

alubbe commented Jan 25, 2019

koskimas commented Jan 26, 2019

alubbe commented Mar 12, 2019

koskimas commented Mar 13, 2019 •

edited

Loading

wubzz Mar 14, 2019

alubbe Mar 14, 2019

wubzz Mar 15, 2019

alubbe Mar 15, 2019 •

edited

Loading

elhigu Mar 16, 2019 •

edited

Loading

alubbe Mar 18, 2019

alubbe Mar 18, 2019

wubzz Mar 14, 2019

alubbe Mar 14, 2019

elhigu Mar 16, 2019 •

edited

Loading

elhigu Mar 16, 2019

alubbe Mar 18, 2019

alubbe commented Mar 18, 2019

elhigu Mar 19, 2019

elhigu Mar 28, 2019

elhigu left a comment

alubbe commented Mar 20, 2019

alubbe commented Mar 28, 2019

elhigu Mar 28, 2019

elhigu commented Mar 28, 2019

alubbe commented Mar 31, 2019

elhigu commented Mar 31, 2019

alubbe commented Apr 1, 2019

Handle opt.destroy() being a promise #16

Handle opt.destroy() being a promise #16

Conversation

alubbe commented Jan 21, 2019

alubbe commented Jan 21, 2019

koskimas commented Jan 21, 2019

koskimas commented Jan 21, 2019

alubbe commented Jan 21, 2019 • edited Loading

koskimas commented Jan 21, 2019

alubbe commented Jan 21, 2019

alubbe commented Jan 25, 2019

koskimas commented Jan 26, 2019

alubbe commented Mar 12, 2019

koskimas commented Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alubbe Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

elhigu Mar 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elhigu Mar 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alubbe commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elhigu left a comment

Choose a reason for hiding this comment

alubbe commented Mar 20, 2019

alubbe commented Mar 28, 2019

Choose a reason for hiding this comment

elhigu commented Mar 28, 2019

alubbe commented Mar 31, 2019

elhigu commented Mar 31, 2019

alubbe commented Apr 1, 2019

alubbe commented Jan 21, 2019 •

edited

Loading

koskimas commented Mar 13, 2019 •

edited

Loading

alubbe Mar 15, 2019 •

edited

Loading

elhigu Mar 16, 2019 •

edited

Loading

elhigu Mar 16, 2019 •

edited

Loading