Skip to content

add deferred commands and relative path support for download & upload#267

Open
willr3 wants to merge 1 commit intoHyperfoil:masterfrom
willr3:defer_cmd
Open

add deferred commands and relative path support for download & upload#267
willr3 wants to merge 1 commit intoHyperfoil:masterfrom
willr3:defer_cmd

Conversation

@willr3
Copy link
Copy Markdown
Collaborator

@willr3 willr3 commented Mar 30, 2026

No description provided.

@willr3 willr3 requested a review from stalep March 30, 2026 18:48
Copy link
Copy Markdown
Member

@stalep stalep left a comment

Choose a reason for hiding this comment

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

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.

@willr3
Copy link
Copy Markdown
Collaborator Author

willr3 commented Apr 9, 2026

  1. Retry loop echo command (Sh.java)

Good catch, I will fix

  1. Removing pwd capture from postRun

This is intentional. Right now we pay the performance price to check the current working directory after every sh. This only makes sense if there is a relative path queue-download observing every sh and only provides a performance gain if there are more than one relative path queue-downloads after every sh.

From my rough "napkin math" the proposed change will be a performance improvement if the ratio of queue-download that use relative paths to sh is less than 1:1. Put another way, this should improve performance for scripts with fewer relative path queue-download than sh

  1. Skipping the entire exit-code block when !shouldCheckExit

Theoretically the flushAndResetBuffer() should not be needed because the buffer ended on the prompt or an abort but in reality some commands have dangling references to the standard out / err. I will move the flush to always run.

@stalep
Copy link
Copy Markdown
Member

stalep commented Apr 9, 2026

Great, I think this is good once those two fixes are pushed! 👍

@willr3
Copy link
Copy Markdown
Collaborator Author

willr3 commented Apr 10, 2026

  1. Retry loop echo command (Sh.java)

Good catch, I will fix

  1. Removing pwd capture from postRun

This is intentional. Right now we pay the performance price to check the current working directory after every sh. This only makes sense if there is a relative path queue-download observing every sh and only provides a performance gain if there are more than one relative path queue-downloads after every sh.

From my rough "napkin math" the proposed change will be a performance improvement if the ratio of queue-download that use relative paths to sh is less than 1:1. Put another way, this should improve performance for scripts with fewer relative path queue-download than sh

  1. Skipping the entire exit-code block when !shouldCheckExit

Theoretically the flushAndResetBuffer() should not be needed because the buffer ended on the prompt or an abort but in reality some commands have dangling references to the standard out / err. I will move the flush to always run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants