Conversation
Martinski4GitHub
commented
Dec 18, 2023
- Code improvements to wait for the F/W update to complete. Instead of an estimated hard-coded 180-second sleep wait, we call the "wait" cmd for the process to actually finish & return, and then wait 30 secs. for the router to reboot itself.
- Removed color escape sequences from the msg string that goes into the log file.
- Trivial formatting changes.
- Code improvements to wait for the F/W update to complete. Instead of an estimated hard-coded 180-second sleep wait, we call the "wait" cmd for the process to actually finish & return, and then wait 30 secs. for the router to reboot itself. - Removed color escape sequences from the msg string that goes into the log file. - Trivial formatting changes.
| -F "file=@${firmware_file}" \ | ||
| --cookie /tmp/cookie.txt > /tmp/upload_response.txt 2>&1 & | ||
| sleep 180 | ||
| curlPID=$! ; wait $curlPID |
There was a problem hiding this comment.
Smart, my only concern if what if the curlPID is frozen or never "completes"?
There was a problem hiding this comment.
We could implement a timeout mechanism to ensure that the script doesn't wait indefinitely?
This can be done using timeout logic... For example, we could do something like this:
curlPID=$!
TIMEOUT=180 # Timeout in seconds (3 minutes in this case)
START_TIME=$(date +%s)
while [ $(($(date +%s) - $START_TIME)) -lt $TIMEOUT ]; do
if ! kill -0 $curlPID 2> /dev/null; then
# curl process is no longer running
break
fi
sleep 5
done
if kill -0 $curlPID 2> /dev/null; then
echo "Curl command timed out, terminating..."
kill $curlPID
fi
/sbin/service reboot
This periodically checks the status of the process in a loop, with a sleep interval.
If the process is still running after a certain amount of time, you kill it and handle the situation as a timeout.
There was a problem hiding this comment.
I'm not sure how I feel about adding so much complexity post-curl.
I do like your method because it's less invasive, if it reboots post-curl on it's own, so be it. Otherwise you wait and the logic for the wait is simple.
My method add complexity post-curl, but also could potentially save us if lets say, the curl started, and froze before completion, or never "completed" and causes the script to hang indefinitely, waiting for a process to finish that never will.
Food for thought. For now I've merged this in.
There was a problem hiding this comment.
@Martinski4GitHub Let me know what your thoughts are. It's likely that I'm over thinking this, what is the likeliness the curl will freeze? Especially considering we have better memory handling now. If we saw signs of freezing we could up the overhead percentage first, to make sure it has enough memory pre-flash, and if that still wasn't enough we could then implement something like my method above if absolutely required, but go my route above only if required.
There was a problem hiding this comment.
You've got a very good point about the possibility that the curl command might get "stuck" and never complete the F/W Update for some reason. It's likely an extremely rare occurrence but it's not zero so we could add a very simple background task as an "escape hatch" to handle such a case. I'll submit a PR with the additonal code so you can review it and see how you feel about it. It's a much simpler solution because the background task virtually does nothing while the F/W Update is in progress, and when it finally "wakes up" it checks if the curl process still exists and kills it, effectively ending the "stuck" process.