Skip to content
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

Better concurrency handling for search JS script loading #118984

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Dec 15, 2023

As discussed in #118961.

Does it solve your issue @notriddle, or did I completely misunderstand?

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez force-pushed the better-async-handling-js branch from 426cd40 to cb24da4 Compare December 15, 2023 15:31
@notriddle
Copy link
Contributor

That works, and I think you did understand the problem, but it seems even more complicated than what we're doing now. It still involves storing the searchIndex in a global variable, and it's still takes a slightly different code path depending on which order the files load in, so there's an opportunity for future changes to add a race condition.

The code in master, right now, works fine. This doesn't seem any more robust or otherwise better.

@GuillaumeGomez
Copy link
Member Author

Agreed. I'll close this PR and still send a cleanup I discovered.

@GuillaumeGomez GuillaumeGomez deleted the better-async-handling-js branch December 15, 2023 15:43
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_a3c2982c-474b-4483-a5bc-a37526453d61
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=better-async-handling-js
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_a3c2982c-474b-4483-a5bc-a37526453d61
GITHUB_REF=refs/pull/118984/merge
GITHUB_REF_NAME=118984/merge
GITHUB_REF_PROTECTED=false
---
info: ES-Check: there were no ES version matching errors!  🎉
+ eslint -c ../src/librustdoc/html/static/.eslintrc.js ../src/librustdoc/html/static/js/externs.js ../src/librustdoc/html/static/js/main.js ../src/librustdoc/html/static/js/scrape-examples.js ../src/librustdoc/html/static/js/search.js ../src/librustdoc/html/static/js/settings.js ../src/librustdoc/html/static/js/src-script.js ../src/librustdoc/html/static/js/storage.js

/checkout/src/librustdoc/html/static/js/main.js
  191:29  error  Strings must use doublequote  quotes
  191:38  error  Strings must use doublequote  quotes
/checkout/src/librustdoc/html/static/js/search.js
/checkout/src/librustdoc/html/static/js/search.js
  3357:5  error  'runSearchIfFullyLoaded' is not defined  no-undef

✖ 3 problems (3 errors, 0 warnings)
  2 errors and 0 warnings potentially fixable with the `--fix` option.
  local time: Fri Dec 15 15:47:21 UTC 2023
  network time: Fri, 15 Dec 2023 15:47:21 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants