-
Notifications
You must be signed in to change notification settings - Fork 16
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
adds the spawning process manager to run with scissors #158
Conversation
@@ -34,26 +35,32 @@ type Agent struct { | |||
cancelF context.CancelFunc | |||
closing uint32 | |||
ctx context.Context | |||
sigs chan os.Signal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we want to handle signals in the Agent
? It would seemingly make even more sense to support signal handling in no-sandbox mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are handling signals. We weren't before. I'm just not using a channel to do it . In the code prior to this PR, we had the sigs channel but there was nothing sending to it... so basically we had a part of a select that would never fire.
Now instead of using the sigs channel, we're just using the same signal handler pattern we use in the node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node uses a channel to ensure signals are handled serially. We can fix this to make it work like the node does later.
@@ -398,15 +408,42 @@ func (a *Agent) newExecutionProviderParams(req *agentapi.DeployRequest, tmpFile | |||
return params, nil | |||
} | |||
|
|||
func (a *Agent) setupSignalHandlers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar concern regarding use of os.Exit
here in setupSignalHandlers
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need Goexit
here either, although this is interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, my point is, even if there are no defers before this Exit right YET, we shouldn't be using it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me, only nits. That said, I still need to put all the changes from 155 apart to understand the full flow of everthing now, just havent had time yet
@@ -398,15 +408,42 @@ func (a *Agent) newExecutionProviderParams(req *agentapi.DeployRequest, tmpFile | |||
return params, nil | |||
} | |||
|
|||
func (a *Agent) setupSignalHandlers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jordan-rash the runtime Just from a quick glance I can see at least 4 other go routines that would be running at the time From the docs:
This seems like a particularly dangerous way to exit an application. It explicitly doesn't shut the app down or even finish the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the spec suite in a new PR.
No description provided.