add deferred commands and relative path support for download & upload#267
add deferred commands and relative path support for download & upload#267willr3 wants to merge 1 commit intoHyperfoil:masterfrom
Conversation
There was a problem hiding this comment.
1. Retry loop echo command (Sh.java)
In the retry loop, the shSync call has the closing quote wrapping the entire string including the (exit ...):
response = context.getShell().shSync("echo \"${__qdup_ec}; (exit $__qdup_ec)\"");Wouldn't this echo the (exit ...) as literal text rather than executing it? Should the quote end before the semicolon so the exit subshell actually runs?
response = context.getShell().shSync("echo \"${__qdup_ec}\"; (exit $__qdup_ec)");2. Removing pwd capture from postRun
Previously, postRun captured the exit code and pwd together in a single round-trip (echo "${__qdup_ec}:::$(pwd)"). With that removed, commands like QueueDownload now call shSync("pwd") individually when resolving relative paths. Could this become a regression for scripts that frequently use relative paths outside of observer contexts — where before the pwd was essentially free, now each one pays its own round-trip + delay?
3. Skipping the entire exit-code block when !shouldCheckExit
Moving shouldCheckExit(context) up to guard the entire block means that when exit code checking is disabled, no shSync runs at all — which also skips flushAndResetBuffer(). Could this leave stale output in the stream buffer and affect subsequent commands? Previously the exit code was always captured and the buffer always flushed; only the abort-on-non-zero part was conditional on shouldCheckExit.
Good catch, I will fix
This is intentional. Right now we pay the performance price to check the current working directory after every From my rough "napkin math" the proposed change will be a performance improvement if the ratio of
Theoretically the |
|
Great, I think this is good once those two fixes are pushed! 👍 |
Good catch, I will fix
This is intentional. Right now we pay the performance price to check the current working directory after every From my rough "napkin math" the proposed change will be a performance improvement if the ratio of
Theoretically the |
There was a problem hiding this comment.
Exit code clobbered by deferred commands (Sh.java)
runDeferred(output, context) runs before the exit code capture (export __qdup_ec=$?). The deferred commands call shSync("pwd"), shSync("echo ~/"), etc., which overwrite $? on the remote shell. By the time we capture the exit code, it reflects the last deferred shSync — not the original command.
This would silently swallow a non-zero exit code from the original sh: command whenever deferred downloads/uploads are present.
Fix: save $? before running deferred commands:
context.getShell().shSync("export __qdup_ec=$?");
runDeferred(output, context);
// then later just echo the saved variable:
String response = context.getShell().shSync("echo \"${__qdup_ec}\"; (exit $__qdup_ec)");Upload defer condition checks wrong variable (Upload.java)
The defer guard includes populatedPath.endsWith("/"), but populatedPath is the local source file. The shell access (mkdir -p) is needed when the destination ends with /. Should this be populatedDestination.endsWith("/")?
No description provided.