fix(process): proc status lost when streaming

[MiroKaku]

Description

Steps to reproduce:

1. Start any streaming using moonlight.
2. Return to the app selection page.
3. Open the sunshine Web-UI, delete or add any application.
4. Streaming status on moonlight has been lost.

Reason:

  void
  refresh(const std::string &file_name) {
    auto proc_opt = proc::parse(file_name);

    if (proc_opt) {
      proc = std::move(*proc_opt); // <---- !!!! Look here, It is overwritten.
    }
  }

Screenshot

None

Issues Fixed or Closed

None

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Dependency update (updates to dependencies)
• Documentation update (changes to documentation)
• Repository update (changes to repository files, e.g. .github/...)

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ReenigneArcher]

Could you add documentation blocks for the new methods in the header file (not in the cpp file)?

https://docs.lizardbyte.dev/projects/sunshine/en/master/md_third-party_2doxyconfig_2docs_2source__code.html#documentation-blocks

[MiroKaku]

Could you add documentation blocks for the new methods in the header file (not in the cpp file)?

https://docs.lizardbyte.dev/projects/sunshine/en/master/md_third-party_2doxyconfig_2docs_2source__code.html#documentation-blocks

done.

[FrogTheFrog]

I've added some comment style suggestion, based on the feedback I've got from RA in my other commits.

Also I've found some other issues with the env that I would like you to fix. Sorry, that such a simple PR turned out to explode. If you prefer I can also take over the requested changes, just let me know.

[FrogTheFrog]

I strongly suggest to change signature of std::optional<proc::proc_t> parse(const std::string &file_name) to std::optional<std:tuple<std::vector<ctx_t>, boost::process::v1::environment>> parse(const std::string &file_name) and also remove proc_t constructor as it is another disaster waiting to happen (don't forget to initialize int _app_id{0};)

[FrogTheFrog]

After looking into this I have found another bug related to _env. It is used in terminate function, this means we have to preserve the _env until process terminates :/

The moment we call execute we should copy the _env to a member variable _app_env (name is debatable) and replace every usage of _env within execute and terminate with _app_env (except the first one where we copy it of course). Or we could store the new ENV to _current_env and the in execute copy it (_env = _current_env). I prefer the first renaming option, but whatever is easier is ok.

[FrogTheFrog]

Also @ReenigneArcher, why do we use -1 here?

Is 0 not good enough?

[ReenigneArcher]

Not sure exactly where that code comes from, but 0 is the first app.

[FrogTheFrog]

Not sure exactly where that code comes from, but 0 is the first app.

But then we have a lot of places in the code checking running() > 0 to determine if any app is running. Which is incorrect...

We should change the app_id type to std::optional<int> in some other PR...

[ReenigneArcher]

That whole system problem needs to be re-thought. As far as I see, app_id is not even real, it's more like an app index.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[MiroKaku]

I've added some comment style suggestion, based on the feedback I've got from RA in my other commits.

Also I've found some other issues with the env that I would like you to fix. Sorry, that such a simple PR turned out to explode. If you prefer I can also take over the requested changes, just let me know.

I would prefer if you could take over the requested changes. Thank you! 😀

[FrogTheFrog]

@ReenigneArcher I will move some data around without changing logic too much. This should automatically resolve all of the known issues.

[FrogTheFrog]

@MiroKaku Have you perhaps unchecked the checkbox where maintainers or something can push to the branch? If so, can you enable it?

Otherwise I will have to create a fork :/

[ReenigneArcher]

@MiroKaku Have you perhaps unchecked the checkbox where maintainers or something can push to the branch? If so, can you enable it?

Otherwise I will have to create a fork :/

It is checked, but I don't think it allows "Collaborators" to push to.

[FrogTheFrog]

I will create a fork then.