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

Improve error reportability #160

Merged
merged 7 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .github/.markdownlint-cli2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@ globs:
ignores:
- 'node_modules/**/*'
- 'tests/**/*'
# GitHub issues don't quite abide by Markdown standards (e.g. single line
# breaks are reproduced when rendered)
- '.github/ISSUE_TEMPLATE/**/*'
44 changes: 33 additions & 11 deletions .github/ISSUE_TEMPLATE/bug-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,47 @@ assignees: ''

---

### Describe the bug
[//]: # (Hello and thank you for taking the time to submit a bug report! )

A clear and concise description of what the bug is.
[//]: # (To make sure we can resolve this as quickly as possible, please )
[//]: # (carefully fill out the information requested on this template. )

[//]: # (Hint: Lines that look like these are Markdown comments and won't be )
[//]: # ( visible on the issue after you submit it so you don't need to )
[//]: # ( delete them. )

### Description

[//]: # (A clear and concise description of what the bug is and what you )
[//]: # (expected to happen instead. Feel free to include additional material )
[//]: # (such as screenshots if they contribute to the explanation. )

### To Reproduce

Steps to reproduce the behavior:
[//]: # (A list of steps we can follow to consistently reproduce the bug )

### Context

[//]: # (Please fill out the following list )

> My operating system is:



> Running `viv --version` outputs:

```txt
output goes here
```

> I installed Vivify in the following way (e.g. name of the package manager, self-compiled or development mode):


1. Use version ...
2.

### Expected behavior
> This is what happens when I kill any running instance of Vivify with `killall vivify-server` and then run `vivify-server` at the absolute installation path (e.g. at `command -v vivify-server`) instead of `viv` and try to reproduce the problem:

A clear and concise description of what you expected to happen.

### Screenshots

If applicable, add screenshots to help explain your problem.
> In the following I specify if I use any custom configuration, and if so provide it in its entirety including any additional files referenced there (e.g. CSS or JavaScript):

### Additional context

Add any other context about the problem here.
28 changes: 19 additions & 9 deletions src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { router as viewerRouter } from './routes/viewer.js';
import { router as openRouter } from './routes/_open.js';
import { setupSockets } from './sockets.js';
import { urlToPath } from './utils/path.js';
import { handleArgs } from './cli.js';
import { completeStartup, handleArgs } from './cli.js';

const app = express();
app.use(express.json());
Expand Down Expand Up @@ -39,11 +39,21 @@ export const { clientsAt, messageClients, openAndMessage } = setupSockets(
},
);

get(`${address}/health`, async () => {
// server is already running
await handleArgs();
process.exit(0);
}).on('error', () => {
// server is not running so we start it
server.listen(config.port, handleArgs);
});
const openTargetsAndCompleteStartup = handleArgs();
if (openTargetsAndCompleteStartup) {
try {
get(`${address}/health`, async () => {
// server is already running
await openTargetsAndCompleteStartup();
process.exit(0);
}).on('error', () => {
// server is not running so we start it
server.listen(config.port, openTargetsAndCompleteStartup);
});
} catch (error) {
console.log(error);
completeStartup();
}
} else {
completeStartup();
}
61 changes: 32 additions & 29 deletions src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,37 +49,40 @@ const openTarget = async (target: string) => {
}
};

export const handleArgs = async () => {
try {
const args = process.argv.slice(2);
const positionals: string[] = [];
let parseOptions = true;
export const completeStartup = () => {
if (process.env['NODE_ENV'] !== 'development') {
// - viv executable waits for this string and then stops printing
// vivify-server's output and terminates
// - the string itself is not shown to the user
console.log('STARTUP COMPLETE');
}
};

for (let i = 0; i < args.length; i++) {
const arg = args[i];
if (!(arg.startsWith('-') && parseOptions)) {
positionals.push(arg);
continue;
}
switch (arg) {
case '-v':
case '--version':
console.log(`vivify-server ${process.env.VERSION ?? 'dev'}`);
break;
case '--':
parseOptions = false;
break;
default:
console.log(`Unknown option "${arg}"`);
}
export const handleArgs = (): (() => Promise<void>) | undefined => {
const args = process.argv.slice(2);
const positionals: string[] = [];
let parseOptions = true;

for (let i = 0; i < args.length; i++) {
const arg = args[i];
if (!(arg.startsWith('-') && parseOptions)) {
positionals.push(arg);
continue;
}
await Promise.all(positionals.map((target) => openTarget(target)));
} finally {
if (process.env['NODE_ENV'] !== 'development') {
// - viv executable waits for this string and then stops printing
// vivify-server's output and terminates
// - the string itself is not shown to the user
console.log('STARTUP COMPLETE');
switch (arg) {
case '-v':
case '--version':
console.log(`vivify-server ${process.env.VERSION ?? 'dev'}`);
return;
case '--':
parseOptions = false;
break;
default:
console.log(`Unknown option "${arg}"`);
}
}
return async () => {
await Promise.all(positionals.map((target) => openTarget(target)));
completeStartup();
};
};
38 changes: 36 additions & 2 deletions viv
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,19 @@ arguments:
source file
options:
--help show this help message and exit
--version show version information
--version show version information and exit
EOF
}

print_bug_report() {
cat <<EOF
Fatal: vivify-server crashed. Please use the link below to submit a bug report.

The bug report template will help you provide the necessary information and
maybe even find a solution yourself.

https://github.com/jannis-baum/Vivify/issues/new?labels=type%3Abug&template=bug-report.md

EOF
}

Expand All @@ -28,12 +40,34 @@ cleanup() {
trap cleanup EXIT

nohup vivify-server $@ > "$output" 2> /dev/null &
server_pid=$!

monitor_server() {
while true; do
# server process ended
if ! kill -0 $server_pid 2>/dev/null; then
# check if startup was completed successfully, if so we can exit
test -f "$output" || exit 0
grep --quiet "STARTUP COMPLETE" "$output" && exit 0

# if not, the startup failed
print_bug_report
# kill tail from while loop below
pkill -P $$ tail
exit 1
fi
sleep 0.3
done
}
monitor_server &

# print stdout of vivify-server until STARTUP COMPLETE is found
tail -f "$output" | while read line; do
if echo "$line" | grep -q "STARTUP COMPLETE"; then
# server finished starting
if echo "$line" | grep --quiet "STARTUP COMPLETE"; then
pkill -P $$ tail
break
fi

echo "$line"
done