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

onDidEndTaskProcess's callback is not always fired when a task exits quickly #58828

Closed
g-arjones opened this issue Sep 17, 2018 · 23 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug tasks Task system issues terminal Integrated terminal issues verified Verification succeeded
Milestone

Comments

@g-arjones
Copy link
Contributor

  • VSCode Version: 1.27.2
  • OS Version: Ubuntu 16.04

Steps to Reproduce:

  1. Register a callback using vscode.tasks.onDidEndTaskProcess
  2. Execute a process (/bin/false) that returns a non zero code using vscode.tasks.executeTask API
  3. Check event.exitCode passed to the callback (should be non zero)

Does this issue occur when all extensions are disabled?: Yes

@jrieken jrieken assigned dbaeumer and unassigned jrieken Sep 17, 2018
@g-arjones
Copy link
Contributor Author

@jrieken @dbaeumer @bpasero Would anyone confirm this, please?

@dbaeumer
Copy link
Member

dbaeumer commented Sep 24, 2018

This works for me under Windows using the following shell execution exit 1.

task

{
	// See https://go.microsoft.com/fwlink/?LinkId=733558
	// for the documentation about the tasks.json format
	"version": "2.0.0",
	"tasks": [
		{
			"label": "exit",
			"type": "shell",
			"command": "exit 1",
			"problemMatcher": []
		}
	]
}

code:

    vscode.tasks.onDidEndTaskProcess((event) => {
        console.log(`Exit code ${event.exitCode}`);
    });

Output

Exit code 1

@g-arjones
Copy link
Contributor Author

Could you try with a task provided by an extension? Also, I had the issue with a ProcessExecution, not sure if it works with a ShellExecution.... Will have a look

@dbaeumer
Copy link
Member

Same if I contribute the task via a provider.

@dbaeumer dbaeumer added terminal Integrated terminal issues tasks Task system issues bug Issue identified by VS Code Team member as probable bug labels Sep 24, 2018
@dbaeumer
Copy link
Member

I can reproduce under Linux. The reason is that the underlying terminal never signals a terminal.processReady and to not send an unbalance begin/end pair a end process is not signaled either.

@Tyriar any insights why this never fires. Is there a different way for me to know when the process actually started.

@Tyriar
Copy link
Member

Tyriar commented Sep 24, 2018

@alexr00 recently touched this area, a37adb6#diff-dc09948028c646a458fac5f5b5d32276 prevents the process ID promise from firing, but only when the terminal exits immediately.

@g-arjones
Copy link
Contributor Author

but only when the terminal exits immediately.

Interesting, it seems that that is a different bug though (and that my bug report is not as accurate as it should have been).

The problem I was having was that killed processes are reported with exitCode == 0, which is obviously wrong. What I suggested as step to reproduce actually hits the other bug you mention (the callback is not fired at all).

Please, let me know if you guys would like me to open another issue for that...

@alexr00
Copy link
Member

alexr00 commented Sep 24, 2018

@g-arjones , can you share a task definition that you're seeing this with?

@g-arjones
Copy link
Contributor Author

g-arjones commented Sep 25, 2018

@alexr00 You could use this...

  1. If you run the "Quick task" that is provided by that test extension you will notice that onDidEndTaskProcess callback is not fired at all (which is the bug @Tyriar mentions..)

  2. If you run the "Slow task" and send a SIGTERM or SIGKILL to the task's process, you will notice that the callback is called with exitCode === 0, which is not expected (my original bug report).

Let me know if you need further details..

@sergei-dyshel
Copy link

I'm also experiencing that. Had to prepend sleep 0.3 to my shell command (which fails immediately) for the event to be fired.

@g-arjones
Copy link
Contributor Author

@alexr00 Did you get the time to have a look into this? Thank you!

@alexr00
Copy link
Member

alexr00 commented Oct 15, 2018

@g-arjones, I haven't been able to reproduce this with Windows. I'm trying with Linux next.

@alexr00
Copy link
Member

alexr00 commented Oct 15, 2018

@g-arjones, ok I can reproduce with Linux and I'm looking into it.

@g-arjones
Copy link
Contributor Author

@alexr00 thanks a lot!

@alexr00
Copy link
Member

alexr00 commented Oct 15, 2018

The task exit code being 0 on Linux has been occurring since at least the beginning of September(went back and checked) so that isn't related to the change I made. This appears to be an upstream issue since 0 is the exit code that we're getting from node-pty.

@g-arjones
Copy link
Contributor Author

@alexr00 How about the event not being fired if the process ends too quickly?

@alexr00
Copy link
Member

alexr00 commented Oct 16, 2018

It's a different issue, but it also wasn't introduced recently. I'll keep investigating.

alexr00 added a commit that referenced this issue Oct 19, 2018
And that tasks wait for terminal process ready before checking to see if task process ended to fire

Related #58828
@alexr00 alexr00 added this to the October 2018 milestone Oct 19, 2018
@alexr00
Copy link
Member

alexr00 commented Oct 19, 2018

@g-arjones, as you can see I'm working on a change for the exit event not being fired.
For the exit code being 0, I did a bit of research and it looks like the exit code is correctly 0, it's the exit signal that should be set. However, we don't currently expose the exit signal through vscode.tasks.onDidEndTaskProcess. If you want the exit signal (not available on Windows) then feel free to open a separate issue as a feature request 😄

@alexr00 alexr00 changed the title event.exitCode passed to onDidEndTaskProcess's callback is always zero onDidEndTaskProcess's callback is not always fired when a task exits quickly Oct 19, 2018
@g-arjones
Copy link
Contributor Author

as you can see I'm working on a change for the exit event not being fired

Yes, I can see that. Thank you very much for the effort. 😄

I did a bit of research and it looks like the exit code is correctly 0, it's the exit

Strictly speaking, on POSIX systems, if the process is killed there is no exit code (or, to be precise, the exit code portion of the exit status is unset). But the point is: returning 0 as exit code for processes that did not really exit is misleading for extension authors (and would be considered a bug by some, specially given that zero indicates sucess in this context). What shells typically do is return something greater than 127 to indicate that a process was signaled. So, couldn't you do something like that (setting exit code to an arbitrary high number) here? node-pty provides the information you need for that.. On Windows that is a non-issue because the code is never 0 for terminated processes anyway...

/cc @dbaeumer @Tyriar @alexr00

@dbaeumer
Copy link
Member

We could also use null to indicate that the process actually didn't provide an exit code.

@g-arjones
Copy link
Contributor Author

Sounds good...

@alexr00
Copy link
Member

alexr00 commented Oct 25, 2018

@Tyriar, what is your preference on how we handle exit codes?

alexr00 added a commit that referenced this issue Oct 26, 2018
And that tasks wait for terminal process ready before checking to see if task process ended to fire

Related #58828
@alexr00
Copy link
Member

alexr00 commented Oct 29, 2018

New issue for the zero exit code: #62043.

@alexr00 alexr00 closed this as completed Oct 29, 2018
@mjbvz mjbvz added the verified Verification succeeded label Nov 2, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug tasks Task system issues terminal Integrated terminal issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants