twisted / ampoule

Process pool for Twisted, using AMP
Other
12 stars 19 forks source link

Don't return an already-dead child to the ready pool #30

Closed hyperair closed 5 years ago

hyperair commented 5 years ago

A single-worker pool that times out will fail all subsequent tasks currently in queue with errors.ProcessTerminated.

The sequence of events is this:

To avoid that, we should avoid returning the already-dead worker to the pool, and just call stopAWorker(child) instead.

glyph commented 5 years ago

Thanks @hyperair !

glyph commented 5 years ago

(Please feel free to @-mention me when you update this, since my github notifications are beyond redemption, and if a thing has been updated and is ready to merge I'd love to hear about it…)

glyph commented 5 years ago

@hyperair Just checking in - do you think you'll be able to address any of the review feedback here?

hyperair commented 5 years ago

@glyph Sorry for the late reply -- I hadn't noticed your comments until my colleague pointed them out to me. I'll work on applying your suggestions.

One thing though -- regarding the HangForever.responder decorator, I knew about the syntax, but specifically didn't use it to be consistent with the rest of the file. Do you wish for me to use the decorator syntax only in this instance, or to convert the entire file to the decorator syntax?

glyph commented 5 years ago

Do you wish for me to use the decorator syntax only in this instance, or to convert the entire file to the decorator syntax?

Go ahead and use the new syntax in your changes; let's have a separate change which updates the other syntaxes. If you wanted to do that one too that would be great, but is not necessary to merge this :)

glyph commented 5 years ago

Thanks for your response!

hyperair commented 5 years ago

@glyph Well, I think I got everything. Could you have another look please?

hyperair commented 5 years ago

No problem, thanks for merging it.

sfdye commented 5 years ago

@glyph Could a new version be released to include this change? Thanks!

glyph commented 5 years ago

@sfdye it’s on my list, but the list is long:) would you like to become an ampoule release manager?