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

Callback/EOI never returns #96

Closed
cawoodm opened this issue Sep 7, 2019 · 5 comments · Fixed by #116
Closed

Callback/EOI never returns #96

cawoodm opened this issue Sep 7, 2019 · 5 comments · Fixed by #116
Labels
Bug A real bug that should be fix

Comments

@cawoodm
Copy link

cawoodm commented Sep 7, 2019

I have a similar problem to #94 where invoke() does not return. The problem is intermittant. In my application I have 4 instances of a Vue component which each instantiate their own Shell and call Get-Service. As you can see below, 3 of the 4 processes return whilst the 4th (PID 15456) does not. Repeat and only 2 return. About one in 10 times all 4 return.

Here's the code: https://github.com/cawoodm/powershell-servicemanager

image

Could it be that the EOI detection is shaky? How does it work exactly?

@cawoodm
Copy link
Author

cawoodm commented Sep 7, 2019

I think I see the problem - you create a random string called EOI which looks like this EOI_llKGpOg8x and then you write it out using powershell after all other commands [Console]::Out.Write("${EOI}").

The problem is that this line is not always streamed on it's own but sometimes together with the user's commands so _write get's something like this:

[{"Name":"Fax","Status":1,"DisplayName":"Fax","StartType":3,"CanStop":false},{"Name":"SysMain","Status":4,"DisplayName":"SysMain","StartType":2,"CanStop":true},{"Name":"WSearch","Status":4,"DisplayName":"Windows Search","StartType":2,"CanStop":true}]
EOI_llKGpOg8x

But it expects this separately like this:

[{"Name":"Fax","Status":1,"DisplayName":"Fax","StartType":3,"CanStop":false},{"Name":"SysMain","Status":4,"DisplayName":"SysMain","StartType":2,"CanStop":true},{"Name":"WSearch","Status":4,"DisplayName":"Windows Search","StartType":2,"CanStop":true}]
EOI_llKGpOg8x

Whether the output is streamed together or not is up to child_process and out side our control.

So this line in utils.js fails:

if(chunk.compare(this.EOI) === 0) {
   cb();
   return this.emit('EOI');
}

It's doing a direct, absolute compare where it should be looking for the EOI string on a single line.

@cawoodm
Copy link
Author

cawoodm commented Sep 7, 2019

OK, I think we have a fix:

_write(chunk, encoding, cb) {
// HERE
    if(chunk.indexOf(this.EOI) >= 0) {
      this.chunksArray.push(chunk);
      cb();
      return this.emit('EOI');
    }
    this.chunksArray.push(chunk);
    return cb();
  }
  isEmpty() {
    return this.chunksArray.length === 0;
  }
  getContents() {
    return Buffer.concat(this.chunksArray);
  }
  getContentsAsString(encoding = 'utf8') {
// AND HERE
    return Buffer.concat(this.chunksArray).toString(encoding).replace(/\0/g, '').replace(this.EOI,'');
  }

cawoodm added a commit to cawoodm/node-powershell that referenced this issue Sep 7, 2019
davistran86 pushed a commit to davistran86/node-powershell that referenced this issue Oct 4, 2019
@codaamok
Copy link

@cawoodm I'm not sure if that is a fix, e.g. something trivial like printing $host (in PowerShell) results in same issue with the never returning. Also, with your changes in cawoodm@863ef27 a lot of the output is truncated (still using $host as an example).

@cawoodm
Copy link
Author

cawoodm commented Oct 13, 2019

It works for my application, feel free to improve on it.

@rannn505 rannn505 added the Bug A real bug that should be fix label Dec 21, 2019
@rannn505
Copy link
Owner

Fix invoke() promise never resolves bug

rannn505 added a commit that referenced this issue Apr 15, 2020
- Convert code base to Typescript
- Add .CONTIBUTE file to repo
- Improve library errors (fix #92)
- Fix invoke() promise never resolves bug (fix #94, fix #96)
- Add support to inject environment variables via constructor options
@rannn505 rannn505 mentioned this issue Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A real bug that should be fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants