add WaitForParentExit to Upgrader

cloudflare / tableflip

Graceful process restarts in Go

BSD 3-Clause "New" or "Revised" License

2.91k stars 148 forks source link

add WaitForParentExit to Upgrader #19

Closed zhiqiangxu closed 5 years ago

zhiqiangxu commented 5 years ago

So that the child process can do something when it's sure that parent has exited.

zhiqiangxu commented 5 years ago

The use case is that I want to do something after I know exactly that parent has quit, not when both parent and child are alive, in order to avoid inconsistent behavior:)

lmb commented 5 years ago

The use case is that I want to do something after I know exactly that parent has quit, not when both parent and child are alive, in order to avoid inconsistent behavior :)

Well... yes :D I guess my question is: how are you going to react when the parent doesn't exit? There is no mechanism where the child kills it's parent after all. I'm sure you have a good reason to do this, but I think it's a fairly complex change.

Thinking about your approach more:

What should happen when Stop() is called? Should Wait... return (if yes, what)?
As it stands, Upgrade() will return a confusing "upgrade in progress" error when something else is waiting for the parent, which isn't good
The u.parentErr race condition is still there (you can't read or write without upgradeSem held)

zhiqiangxu commented 5 years ago

My use case is for im server. During upgrade, users are split between old and new process, so I want to disable messaging during it,otherwise the result may be inconsistent, like some are notified but some are not.

I'll fix the first two issues tomorrow as it's very late in China:)

About the 3rd one,I think it's fine since parentErr is only ever set once, it's like how we implement sington method:)

lmb commented 5 years ago

Can you take a look at https://github.com/cloudflare/tableflip/commit/5c32c1fae14f23a753b0b6f674f41682b1edfdd2 and let me know whether that would be fit for your purpose?

zhiqiangxu commented 5 years ago

Sorry too busy these days! I can't judge whether it's correct based on the commit, as it seems u.parent.result is newly added? The bad news is that I finally used another solution for my IM server that doesn't require WaitForParent ...

lmb commented 5 years ago

Yes, there is another commit in the branch which refactors the upgrader.

On Thu, 31 Jan 2019 at 03:23, zhiqiangxu notifications@github.com wrote:

Sorry too busy these days! I can't judge whether it's correct based on the commit, as it seems u.parent.result is newly added?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cloudflare/tableflip/pull/19#issuecomment-459200760, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWfCCDkSdPszXvU5D8Nlcdx3EWfaHpcks5vImHMgaJpZM4aL7AG .

-- Lorenz Bauer | Systems Engineer 25 Lavington St., London SE1 0NZ

www.cloudflare.com

lmb commented 5 years ago

Please have a look at 1555bf56e377b03d4d1b34410c62c9ec9aff12b9 which should do what you want.