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

Remove process-exit event (function --on-process) #4700

Closed
faho opened this issue Feb 3, 2018 · 4 comments
Closed

Remove process-exit event (function --on-process) #4700

faho opened this issue Feb 3, 2018 · 4 comments

Comments

@faho
Copy link
Member

faho commented Feb 3, 2018

As we've seen in #4582, there's a few issues with the process-exit event.

This thing:

function on_exit --on-process $PID
    # do stuff
end
  • It currently hangs WSL if fish is configured as the login shell (since it gets PID 2) - which we can fix

  • It is only triggered for the leading process of a job - which we can fix, but that would then send more events that nobody listens to

But more importantly: It is fundamentally broken. It requires either getting the PID from fish - in which case one could just use the job-id instead - or guessing the PID (which does not work). It can also only ever work for fish's children.

It seems like this is only ever used to do stuff when fish exits (--on-process %self - also in OMF and fisherman repositories), so I would propose to make that explicit.

So here's what I'm planning to do:

  • Remove the general "EVENT_EXIT" event - in the future, no need to rush this

  • Introduce a more specific "--on-fish-exit" switch and corresponding event (or just simply a "fish_exit" event)

@faho faho added this to the fish-3.0 milestone Feb 3, 2018
@faho
Copy link
Member Author

faho commented Feb 3, 2018

It is only triggered for the leading process of a job - which we can fix, but that would then send more events that nobody listens to

This seems to not actually be true. proc.cpp:process_clean_after_marking() has two places where it's firing an "EVENT_EXIT" event. Once when cleaning up the process (which does use the PID), and once for the finished job in its entirety.

I can't actually get the latter to trigger (when I define an exit handler, I always get "PROCESS_EXIT", not "JOB_EXIT" in $argv). Which is annoying, since that's the one causing all those issues with WSL.

@ridiculousfish
Copy link
Member

I'm not sure I follow - is the idea to get rid of process death notifications because job death notifications are sufficient?

@faho
Copy link
Member Author

faho commented Feb 4, 2018

Basically, yes. Process notifications provide nothing of value over job notifications.

Both are a bit awkward to use (because you need to add this when defining the function, so you need to define a function for every time you want to use this, and after you have started the job), actually, but process notifications are even weirder.

The theoretical advantage of process notification (that I can react to one particular process in a job completing, and potentially earlier) is outweighed by it looking like it can do stuff it can't (I can't react to my firefox closing because that's not a child of fish) and by being even awkwarder to use if I actually want to do something to one particular process (since I'd have to use jobs -p, and then have to count which PID it is).

@ridiculousfish
Copy link
Member

ridiculousfish commented Feb 17, 2018

Sorry for the delay - go for it if you want! LGTM

faho added a commit to faho/fish-shell that referenced this issue Feb 19, 2018
Turns out the process-exit is only ever used in conjunction with
`%self`. Make that explicit by just adding a new "fish_exit" event,
and deprecate the general process-exit machinery.

Fixes fish-shell#4700.
@faho faho closed this as completed in b67a614 Feb 19, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants