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

fix: tsconfig self-references cause short-lived invaild cache #210

Closed
wants to merge 1 commit into from

Conversation

jerrykingxyz
Copy link
Contributor

@jerrykingxyz jerrykingxyz commented Jul 1, 2024

The resolver will load references and write them to cache. However, if the resolver receives a new request when self_reference is saved to the cache but other reference not finish , the new request will fail.

Here is a simple example

Resolver::new(ResolveOptions {
    tsconfig: Some(TsconfigOptions {
        config_file: PathBuf::from("/apps/a/tsconfig.json"),
        references: TsconfigReferences::Paths(vec![
            "/apps/a/tsconfig.json".into(),
            "/apps/b".into(), // mark 1
            "/apps/c".into()
        ])
    })
})
  1. The first request comes, and the resolver executes to mark1.
    The tsconfig cache will contains /apps/a/tsconfig.json which insert by

    let tsconfig = self.cache.tsconfig(

  2. The next request comes, and the resolver will use the cached tsconfig which is no tsconfig info. This request will be failed

@Boshen
Copy link
Member

Boshen commented Jul 1, 2024

tsconfigs: DashMap<PathBuf, Arc<TsConfig>, BuildHasherDefault<FxHasher>>,

This shouldn't be a Dashmap, loading tsconfig should block the thread.

@Boshen
Copy link
Member

Boshen commented Jul 1, 2024

Let me figure out how to setup a test for this first ...

@Boshen
Copy link
Member

Boshen commented Jul 1, 2024

I can't accept this PR without a failing test case, here's what I tried with no luck:

in src/tests/mod.rs:

#[test]
fn race_condition() {
    use rayon::iter::{IntoParallelRefIterator, ParallelIterator};

    use crate::{ResolveOptions, Resolver, TsconfigOptions, TsconfigReferences};
    let f = fixture_root().join("tsconfig/cases/project_references");

    let resolver = Arc::new(Resolver::new(ResolveOptions {
        tsconfig: Some(TsconfigOptions {
            config_file: f.join("app/tsconfig.json"),
            references: TsconfigReferences::Paths(vec![
                "../project_b".into(),
                "../project_c/tsconfig.json".into(),
                "../../paths_template_variable/tsconfig2.json".into(),
                "../project_a/conf.json".into(),
            ]),
        }),
        ..ResolveOptions::default()
    }));

    #[rustfmt::skip]
    let paths =[
        (f.clone(), "./app/tsconfig.json",f.join("app/tsconfig.json")),
        (f.clone(), "./project_a/conf.json",f.join("project_a/conf.json")),
    ];
    let paths = paths.iter().cycle().take(10000).collect::<Vec<_>>();
    paths.par_iter().for_each_with(resolver, |resolver, (path, request, expected)| {
        let resolved_path = resolver.resolve(&path, request).map(|f| f.full_path());
        assert_eq!(resolved_path, Ok(expected.clone()), "{request} {path:?}");
    });
}

Is this a race condition bug or logic bug? I'm confused 🤔

Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

Request failing test.

@Boshen
Copy link
Member

Boshen commented Jul 1, 2024

Oh I got a failing test, there is a self-reference.

@Boshen
Copy link
Member

Boshen commented Jul 1, 2024

I think this is a logic bug?

config_file: PathBuf::from("/apps/a/tsconfig.json"),
        references: TsconfigReferences::Paths(vec![
            "/apps/a/tsconfig.json".into(),

Does this even make sense? A tsconfig that project references it self? Should this throw an error instead?

@jerrykingxyz
Copy link
Contributor Author

Yes, maybe a throw an error is better. I will try to update the PR.

@Boshen
Copy link
Member

Boshen commented Jul 1, 2024

Test can be added to src/tests/tsconfig_project_references.rs

As for the error message, you probably wanna take a look at what TypeScript throws.

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