Skip to content

fix(process): proc status lost when streaming #3417

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.

Sign up for GitHub

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

Closed
MiroKaku wants to merge 10 commits into from
Closed

fix(process): proc status lost when streaming #3417

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
327ce77
fix(process): proc status lost when streaming
MiroKaku Nov 25, 2024
dae0997
Merge branch 'master' into fix/process/proc_status_lost
MiroKaku Dec 1, 2024
c70404a
Merge branch 'master' into fix/process/proc_status_lost
MiroKaku Dec 8, 2024
fb78d68
merge master
MiroKaku Jan 21, 2025
c53681a
docs(process): add comments
MiroKaku Jan 21, 2025
1352925
merge master: rollback
MiroKaku Jan 21, 2025
04947f3
Merge branch 'master' into fix/process/proc_status_lost
MiroKaku Jan 22, 2025
06ad747
style: process.cpp
MiroKaku Jan 22, 2025
6a1465b
fix(process): typo
MiroKaku Jan 23, 2025
56d0699
Merge branch 'master' into fix/process/proc_status_lost
MiroKaku Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,10 @@ namespace proc {
proc_t::get_apps() {
return _apps;
}
void
proc_t::set_apps(std::vector<ctx_t> apps) {
_apps = std::move(apps);
}

// Gets application image from application list.
// Returns image from assets directory if found there.
Expand All @@ -382,6 +386,19 @@ namespace proc {
return _app.name;
}

const boost::process::v1::environment &
proc_t::get_env() const {
return _env;
}
boost::process::v1::environment &
proc_t::get_env() {
return _env;
}
void
proc_t::set_env(boost::process::v1::environment env) {
_env = std::move(env);
}

proc_t::~proc_t() {
// It's not safe to call terminate() here because our proc_t is a static variable
// that may be destroyed after the Boost loggers have been destroyed. Instead,
Expand Down Expand Up @@ -716,7 +733,8 @@ namespace proc {
auto proc_opt = proc::parse(file_name);
Copy link
Copy Markdown
Contributor

@FrogTheFrog FrogTheFrog Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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};)


if (proc_opt) {
proc = std::move(*proc_opt);
proc.set_env(proc_opt->get_env());
Copy link
Copy Markdown
Contributor

@FrogTheFrog FrogTheFrog Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

proc.set_apps(proc_opt->get_apps());
}
}
} // namespace proc
8 changes: 8 additions & 0 deletions src/process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,18 @@ namespace proc {
get_apps() const;
std::vector<ctx_t> &
get_apps();
void
set_apps(std::vector<ctx_t> apps);
std::string
get_app_image(int app_id);
std::string
get_last_run_app_name();
const boost::process::v1::environment &
get_env() const;
boost::process::v1::environment &
get_env();
void
set_env(boost::process::v1::environment env);
void
terminate();

Expand Down