Skip to content

Deno.env review/cleanup: no set() past startup #12621

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cscheid
Copy link
Collaborator

@cscheid cscheid commented Apr 24, 2025

We need to get our Deno.env.* act together. It's generally not safe to call Deno.env.set past the start of the process, because Deno.env is global state which will be the cause of many race conditions from async code. This will become more acute when we add parallelism to Quarto, but it's already a problem in the test suite.

@MichaelHatherly This PR affects all engines, and so we will need a bit of help from you on the julia side - ExecuteOptions now has a new env field that needs to be communicated to the execution processes appropriately. You shouldn't assume that the environment will be the same from one call to another, and so this will need proper handling on your side.

We hope to merge this relatively early on in 1.8.

@cscheid cscheid added this to the v1.8 milestone Apr 24, 2025
@posit-snyk-bot
Copy link
Collaborator

posit-snyk-bot commented Apr 24, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

@MichaelHatherly
Copy link
Contributor

Sorry, only just seeing this notification now for some reason. Just let me know how you'd like to coordinate any changes in QNR.

@cscheid
Copy link
Collaborator Author

cscheid commented May 13, 2025

@MichaelHatherly Ok, great! I'll reach out when everything else appears to be set up and working

@cscheid
Copy link
Collaborator Author

cscheid commented May 21, 2025

@MichaelHatherly I've added a minimal test for the julia engine environment change in tests/docs/smoke-all/engine/env/julia.qmd.

The specific change we made is that ExecuteOptions now has an env field:

env: Record<string, string>;

This environment is communicated to engines in the execute method, but I haven't made the required changes to the julia engine.

What's your preferred way to coordinate? The easiest way is likely for you to work on a branch off of bugfix/no-env-set-past-startup. Alternatively, we merge this change as is, knowing that our test suite won't pass, and wait for your PR. I'd prefer to not have breakage on main, but this is a special situation.

@MichaelHatherly
Copy link
Contributor

So if I understand correctly now quarto manually creates a dictionary with the env vars it wants to send to the QNR server process as part of the JSON payload rather than passing them via actual env vars which was kind of a side-channel? Is that roughly right? If so it's probably just a small change needed in QNR to support this which I can sort out today.

@cscheid
Copy link
Collaborator Author

cscheid commented May 21, 2025

So if I understand correctly now quarto manually creates a dictionary with the env vars it wants to send to the QNR server process as part of the JSON payload rather than passing them via actual env vars which was kind of a side-channel? Is that roughly right? If so it's probably just a small change needed in QNR to support this which I can sort out today.

Yes, that's correct. The old way of doing it is not safe under async calls. Imagine two documents being rendered asynchronously with different environments (and QUARTO_DOCUMENT_FILE guarantees that different documents will always have different environments). Instead, we now communicate the required environment for the engines, and the engines are responsible for setting that appropriately.

@MichaelHatherly
Copy link
Contributor

MichaelHatherly commented May 21, 2025

Thanks. I've pulled this branch and stuck some logging in QNR to find where those env vars are in the JSON payload on the Julia side of things. All I got was:

{
    "format": {
        "pandoc": {
            "to": "html"
        },
        "execute": {
            "daemon": null,
            "fig-width": 7,
            "cache": true,
            "fig-height": 5,
            "error": false,
            "fig-dpi": 96,
            "eval": true,
            "fig-format": "retina"
        },
        "metadata": {
            "julia": {
                "exeflags": [
                ],
                "env": [
                ]
            }
        }
    },
    "params": {
    }
}

Where are the env vars expected to be?

@cscheid
Copy link
Collaborator Author

cscheid commented May 21, 2025

I don't know who's creating that JSON payload, but if you print out the options parameter from src/execute/julia.ts, you will see the env field:

{
  "target": {
    "source": "julia.qmd",
    "input": "julia.qmd",
    "markdown": {
      "value": "---\nformat: html\n_quarto:\n  tests:\n    html:\n      ensureFileRegexMatches:\n        - [\"julia.qmd\"]\n        - []\nengine: julia\n---\n\n```{julia}\nENV[\"QUARTO_DOCUMENT_FILE\"]\n```"
    },
    "metadata": {
      "format": "html",
      "_quarto": {
        "tests": {
          "html": {
            "ensureFileRegexMatches": [
              [
                "julia.qmd"
              ],
              []
            ]
          }
        }
      },
      "engine": "julia"
    }
  },
  "resourceDir": "/Users/cscheid/repos/github/quarto-dev/quarto-cli/src/resources/",
  "tempDir": "/var/folders/nm/m64n9_z9307305n0xtzpp54m0000gn/T/quarto-sessionf4ffc015609a1bf1/2a71403163d4cff/2c07530c4a2982bc",
  "dependencies": true,
  "libDir": "julia_files/libs",
  "format": {
    "identifier": {
      "display-name": "HTML",
      "target-format": "html",
      "base-format": "html"
    },
    "execute": {
      "fig-width": 7,
      "fig-height": 5,
      "fig-format": "retina",
      "fig-dpi": 96,
      "df-print": "default",
      "error": false,
      "eval": true,
      "cache": null,
      "freeze": false,
      "echo": true,
      "output": true,
      "warning": true,
      "include": true,
      "keep-md": false,
      "keep-ipynb": false,
      "ipynb": null,
      "enabled": null,
      "daemon": null,
      "daemon-restart": false,
      "debug": false,
      "ipynb-filters": [],
      "ipynb-shell-interactivity": null,
      "plotly-connected": true,
      "engine": "julia"
    },
    "render": {
      "keep-tex": false,
      "keep-typ": false,
      "keep-source": false,
      "keep-hidden": false,
      "prefer-html": false,
      "output-divs": true,
      "output-ext": "html",
      "fig-align": "default",
      "fig-pos": null,
      "fig-env": null,
      "code-fold": "none",
      "code-overflow": "scroll",
      "code-link": false,
      "code-line-numbers": false,
      "code-tools": false,
      "tbl-colwidths": "auto",
      "merge-includes": true,
      "inline-includes": false,
      "preserve-yaml": false,
      "latex-auto-mk": true,
      "latex-auto-install": true,
      "latex-clean": true,
      "latex-min-runs": 1,
      "latex-max-runs": 10,
      "latex-makeindex": "makeindex",
      "latex-makeindex-opts": [],
      "latex-tlmgr-opts": [],
      "latex-input-paths": [],
      "latex-output-dir": null,
      "link-external-icon": false,
      "link-external-newwindow": false,
      "self-contained-math": false,
      "format-resources": [],
      "notebook-links": true
    },
    "pandoc": {
      "standalone": true,
      "wrap": "none",
      "default-image-extension": "png",
      "to": "html",
      "output-file": "julia.html"
    },
    "language": {
      "toc-title-document": "Table of contents",
      "toc-title-website": "On this page",
      "related-formats-title": "Other Formats",
      "related-notebooks-title": "Notebooks",
      "source-notebooks-prefix": "Source",
      "other-links-title": "Other Links",
      "code-links-title": "Code Links",
      "launch-dev-container-title": "Launch Dev Container",
      "launch-binder-title": "Launch Binder",
      "article-notebook-label": "Article Notebook",
      "notebook-preview-download": "Download Notebook",
      "notebook-preview-download-src": "Download Source",
      "notebook-preview-back": "Back to Article",
      "manuscript-meca-bundle": "MECA Bundle",
      "section-title-abstract": "Abstract",
      "section-title-appendices": "Appendices",
      "section-title-footnotes": "Footnotes",
      "section-title-references": "References",
      "section-title-reuse": "Reuse",
      "section-title-copyright": "Copyright",
      "section-title-citation": "Citation",
      "appendix-attribution-cite-as": "For attribution, please cite this work as:",
      "appendix-attribution-bibtex": "BibTeX citation:",
      "appendix-view-license": "View License",
      "title-block-author-single": "Author",
      "title-block-author-plural": "Authors",
      "title-block-affiliation-single": "Affiliation",
      "title-block-affiliation-plural": "Affiliations",
      "title-block-published": "Published",
      "title-block-modified": "Modified",
      "title-block-keywords": "Keywords",
      "callout-tip-title": "Tip",
      "callout-note-title": "Note",
      "callout-warning-title": "Warning",
      "callout-important-title": "Important",
      "callout-caution-title": "Caution",
      "code-summary": "Code",
      "code-tools-menu-caption": "Code",
      "code-tools-show-all-code": "Show All Code",
      "code-tools-hide-all-code": "Hide All Code",
      "code-tools-view-source": "View Source",
      "code-tools-source-code": "Source Code",
      "tools-share": "Share",
      "tools-download": "Download",
      "code-line": "Line",
      "code-lines": "Lines",
      "copy-button-tooltip": "Copy to Clipboard",
      "copy-button-tooltip-success": "Copied!",
      "repo-action-links-edit": "Edit this page",
      "repo-action-links-source": "View source",
      "repo-action-links-issue": "Report an issue",
      "back-to-top": "Back to top",
      "search-no-results-text": "No results",
      "search-matching-documents-text": "matching documents",
      "search-copy-link-title": "Copy link to search",
      "search-hide-matches-text": "Hide additional matches",
      "search-more-match-text": "more match in this document",
      "search-more-matches-text": "more matches in this document",
      "search-clear-button-title": "Clear",
      "search-text-placeholder": "",
      "search-detached-cancel-button-title": "Cancel",
      "search-submit-button-title": "Submit",
      "search-label": "Search",
      "toggle-section": "Toggle section",
      "toggle-sidebar": "Toggle sidebar navigation",
      "toggle-dark-mode": "Toggle dark mode",
      "toggle-reader-mode": "Toggle reader mode",
      "toggle-navigation": "Toggle navigation",
      "crossref-fig-title": "Figure",
      "crossref-tbl-title": "Table",
      "crossref-lst-title": "Listing",
      "crossref-thm-title": "Theorem",
      "crossref-lem-title": "Lemma",
      "crossref-cor-title": "Corollary",
      "crossref-prp-title": "Proposition",
      "crossref-cnj-title": "Conjecture",
      "crossref-def-title": "Definition",
      "crossref-exm-title": "Example",
      "crossref-exr-title": "Exercise",
      "crossref-ch-prefix": "Chapter",
      "crossref-apx-prefix": "Appendix",
      "crossref-sec-prefix": "Section",
      "crossref-eq-prefix": "Equation",
      "crossref-lof-title": "List of Figures",
      "crossref-lot-title": "List of Tables",
      "crossref-lol-title": "List of Listings",
      "environment-proof-title": "Proof",
      "environment-remark-title": "Remark",
      "environment-solution-title": "Solution",
      "listing-page-order-by": "Order By",
      "listing-page-order-by-default": "Default",
      "listing-page-order-by-date-asc": "Oldest",
      "listing-page-order-by-date-desc": "Newest",
      "listing-page-order-by-number-desc": "High to Low",
      "listing-page-order-by-number-asc": "Low to High",
      "listing-page-field-date": "Date",
      "listing-page-field-title": "Title",
      "listing-page-field-description": "Description",
      "listing-page-field-author": "Author",
      "listing-page-field-filename": "File Name",
      "listing-page-field-filemodified": "Modified",
      "listing-page-field-subtitle": "Subtitle",
      "listing-page-field-readingtime": "Reading Time",
      "listing-page-field-wordcount": "Word Count",
      "listing-page-field-categories": "Categories",
      "listing-page-minutes-compact": "{0} min",
      "listing-page-category-all": "All",
      "listing-page-no-matches": "No matching items",
      "listing-page-words": "{0} words",
      "listing-page-filter": "Filter",
      "draft": "Draft"
    },
    "metadata": {
      "lang": "en",
      "fig-responsive": true,
      "quarto-version": "99.9.9",
      "format": {
        "html": {}
      },
      "_quarto": {
        "tests": {
          "html": {
            "ensureFileRegexMatches": [
              [
                "julia.qmd"
              ],
              []
            ]
          }
        }
      }
    },
    "extensions": {
      "book": {
        "multiFile": true
      }
    }
  },
  "projectDir": "/Users/cscheid/repos/github/quarto-dev/quarto-cli/tests/docs/smoke-all/engine/env",
  "cwd": "/Users/cscheid/repos/github/quarto-dev/quarto-cli/tests/docs/smoke-all/engine/env",
  "handledLanguages": [
    "mermaid",
    "dot"
  ],
  "project": {
    "dir": "/Users/cscheid/repos/github/quarto-dev/quarto-cli/tests/docs/smoke-all/engine/env",
    "engines": [],
    "files": {
      "input": []
    },
    "notebookContext": {},
    "fileInformationCache": {},
    "isSingleFile": true,
    "diskCache": {
      "projectScratchDir": "/var/folders/nm/m64n9_z9307305n0xtzpp54m0000gn/T/quarto-sessionf4ffc015609a1bf1/476194c8bf0de1ac/c1ab9b49c0ce2b92",
      "index": {}
    },
    "temp": {
      "baseDir": "/var/folders/nm/m64n9_z9307305n0xtzpp54m0000gn/T/quarto-sessionf4ffc015609a1bf1/476194c8bf0de1ac"
    },
    "brandCache": {}
  },
  "env": {
    "QUARTO_PROJECT_ROOT": "/Users/cscheid/repos/github/quarto-dev/quarto-cli/tests/docs/smoke-all/engine/env",
    "QUARTO_DOCUMENT_PATH": ".",
    "QUARTO_DOCUMENT_FILE": "julia.qmd"
  }
}

Somehow whoever creates that payload needs to send along options.env, and I think it'll all work itself out.

@MichaelHatherly
Copy link
Contributor

Somehow whoever creates that payload needs to send along options.env, and I think it'll all work itself out.

Thanks for the hint. That's QNR itself that creates it so probably just need to adjust some paths. We had picked out only the keys we really needed to avoid passing too much along internally. I think I know where to look now.

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.

3 participants