Skip to content

runner.pm: apply minor correctness fix#21672

Closed
vszakats wants to merge 1 commit into
curl:masterfrom
vszakats:runnerpm-llm
Closed

runner.pm: apply minor correctness fix#21672
vszakats wants to merge 1 commit into
curl:masterfrom
vszakats:runnerpm-llm

Conversation

@vszakats

@vszakats vszakats commented May 19, 2026

Copy link
Copy Markdown
Member

"Lines 244-245 overwrite global variables $runnerr and $runnerw that
were already assigned in the child process (lines 205-206). In the
parent process context, these assignments appear incorrect and could
cause issues if runner_init is called multiple times. The parent
should only store references in the controller hashes."

It could never cause an actual issue, but clarifies the intent of the
code.

Spotted and fixed by GitHub Code Quality

Cherry-picked from #21646


https://github.com/curl/curl/pull/21672/files?w=1

Lines 244-245 overwrite global variables `$runnerr` and `$runnerw` that
were already assigned in the child process (lines 205-206). In the
parent process context, these assignments appear incorrect and could
cause issues if `runner_init` is called multiple times. The parent
should only store references in the controller hashes.

Spotted and fixed by GitHub Code Quality

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR prevents the parent/controller process from overwriting runner-side pipe globals when runners are initialized in multiprocess mode.

Changes:

  • Limits $runnerr and $runnerw assignment to integrated single-process mode.
  • Preserves per-runner controller pipe storage for both modes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dfandrich

Copy link
Copy Markdown
Contributor

"Lines 244-245 overwrite global variables $runnerr and $runnerw that were already assigned in the child process (lines 205-206). In the parent process context, these assignments appear incorrect

They seem to me to be unnecessary, but harmless.

and could cause issues if runner_init is called multiple times.

I don't see that as ever being an issue. This function isn't casually called to initialize an empty struct but rather to spawn a new worker, which has side effects that would be hard to ignore.

The parent should only store references in the controller hashes."

This patch may help clarify the intent of the code, since $runnerr and $runnerw in the parent are never accessed in $multiprocess mode. So I would be fine to apply it.

Spotted and fixed by GitHub Code Quality

Cherry-picked from #21646

https://github.com/curl/curl/pull/21672/files?w=1

@vszakats vszakats closed this in 68e0b13 May 20, 2026
@vszakats

Copy link
Copy Markdown
Member Author

Thanks for reviewing, Dan. I've merged!

@vszakats vszakats deleted the runnerpm-llm branch May 20, 2026 01:16
outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026
"Lines 244-245 overwrite global variables `$runnerr` and `$runnerw` that
were already assigned in the child process (lines 205-206). In the
parent process context, these assignments appear incorrect and could
cause issues if `runner_init` is called multiple times. The parent
should only store references in the controller hashes."

It could never cause an actual issue, but clarifies the intent of the
code.

Spotted and fixed by GitHub Code Quality

Cherry-picked from curl#21646

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

Development

Successfully merging this pull request may close these issues.

3 participants