Skip to content

Commit

Permalink
[Turbopack] fix require.resolve self hanging (vercel#69793)
Browse files Browse the repository at this point in the history
### What?

evaluation require.resolve tries to resolve and process a module during
module analytics. Since analytics is part of processing a module this
can create a call cycle when `require.resolve("./self")` is used. This
happens in some npm packages and caused Turbopack to hange.

This PR fixes that problem by only resolving the module and not
processing it when using `require.resolve`.

In addition to that it adds a panic when a call cycle occurs. The next
time we mess it up and create a call cycle this will crash Turbopack
instead of hanging, which makes it much easier to find the problem.

fixes PACK-3227
  • Loading branch information
sokra authored Sep 6, 2024
1 parent a9bfc64 commit ec7b33e
Show file tree
Hide file tree
Showing 6 changed files with 156 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ impl<C: AggregationContext> PreparedInternalOperation<C> for PreparedNotifyNewFo
let follower = ctx.node(&follower_id);
let follower_aggregation_number = follower.aggregation_number();
if follower_aggregation_number == upper_aggregation_number {
if upper_id == follower_id {
panic!(
"Cycle in call graph (A function calls itself \
recursively with the same arguments. This will never \
finish and would hang indefinitely.)"
);
}
increase_aggregation_number_internal(
ctx,
balance_queue,
Expand Down
115 changes: 97 additions & 18 deletions turbopack/crates/turbopack-core/src/resolve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2752,10 +2752,6 @@ async fn resolve_package_internal_with_imports_field(
.await
}

async fn is_unresolveable(result: Vc<ModuleResolveResult>) -> Result<bool> {
Ok(*result.resolve().await?.is_unresolveable().await?)
}

pub async fn handle_resolve_error(
result: Vc<ModuleResolveResult>,
reference_type: Value<ReferenceType>,
Expand All @@ -2765,41 +2761,124 @@ pub async fn handle_resolve_error(
severity: Vc<IssueSeverity>,
source: Option<Vc<IssueSource>>,
) -> Result<Vc<ModuleResolveResult>> {
async fn is_unresolveable(result: Vc<ModuleResolveResult>) -> Result<bool> {
Ok(*result.resolve().await?.is_unresolveable().await?)
}
Ok(match is_unresolveable(result).await {
Ok(unresolveable) => {
if unresolveable {
ResolvingIssue {
emit_unresolveable_issue(
severity,
file_path: origin_path,
request_type: format!("{} request", reference_type.into_value()),
origin_path,
reference_type,
request,
resolve_options,
error_message: None,
source,
}
.cell()
.emit();
);
}

result
}
Err(err) => {
ResolvingIssue {
emit_resolve_error_issue(
severity,
file_path: origin_path,
request_type: format!("{} request", reference_type.into_value()),
origin_path,
reference_type,
request,
resolve_options,
error_message: Some(format!("{}", PrettyPrintError(&err))),
err,
source,
}
.cell()
.emit();
);
ModuleResolveResult::unresolveable().cell()
}
})
}

pub async fn handle_resolve_source_error(
result: Vc<ResolveResult>,
reference_type: Value<ReferenceType>,
origin_path: Vc<FileSystemPath>,
request: Vc<Request>,
resolve_options: Vc<ResolveOptions>,
severity: Vc<IssueSeverity>,
source: Option<Vc<IssueSource>>,
) -> Result<Vc<ResolveResult>> {
async fn is_unresolveable(result: Vc<ResolveResult>) -> Result<bool> {
Ok(*result.resolve().await?.is_unresolveable().await?)
}
Ok(match is_unresolveable(result).await {
Ok(unresolveable) => {
if unresolveable {
emit_unresolveable_issue(
severity,
origin_path,
reference_type,
request,
resolve_options,
source,
);
}

result
}
Err(err) => {
emit_resolve_error_issue(
severity,
origin_path,
reference_type,
request,
resolve_options,
err,
source,
);
ResolveResult::unresolveable().cell()
}
})
}

fn emit_resolve_error_issue(
severity: Vc<IssueSeverity>,
origin_path: Vc<FileSystemPath>,
reference_type: Value<ReferenceType>,
request: Vc<Request>,
resolve_options: Vc<ResolveOptions>,
err: anyhow::Error,
source: Option<Vc<IssueSource>>,
) {
ResolvingIssue {
severity,
file_path: origin_path,
request_type: format!("{} request", reference_type.into_value()),
request,
resolve_options,
error_message: Some(format!("{}", PrettyPrintError(&err))),
source,
}
.cell()
.emit();
}

fn emit_unresolveable_issue(
severity: Vc<IssueSeverity>,
origin_path: Vc<FileSystemPath>,
reference_type: Value<ReferenceType>,
request: Vc<Request>,
resolve_options: Vc<ResolveOptions>,
source: Option<Vc<IssueSource>>,
) {
ResolvingIssue {
severity,
file_path: origin_path,
request_type: format!("{} request", reference_type.into_value()),
request,
resolve_options,
error_message: None,
source,
}
.cell()
.emit();
}

// TODO this should become a TaskInput instead of a Vc
/// ModulePart represents a part of a module.
///
Expand Down
8 changes: 4 additions & 4 deletions turbopack/crates/turbopack-ecmascript/src/references/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ use turbopack_core::{
source_map::{GenerateSourceMap, OptionSourceMap, SourceMap},
};
use turbopack_resolve::{
ecmascript::{apply_cjs_specific_options, cjs_resolve},
ecmascript::{apply_cjs_specific_options, cjs_resolve_source},
typescript::tsconfig,
};
use turbopack_swc_utils::emitter::IssueEmitter;
Expand Down Expand Up @@ -2383,14 +2383,14 @@ async fn require_resolve_visitor(
Ok(if args.len() == 1 {
let pat = js_value_to_pattern(&args[0]);
let request = Request::parse(Value::new(pat.clone()));
let resolved = cjs_resolve(origin, request, None, IssueSeverity::Warning.cell())
let resolved = cjs_resolve_source(origin, request, None, IssueSeverity::Warning.cell())
.resolve()
.await?;
let mut values = resolved
.primary_modules()
.primary_sources()
.await?
.iter()
.map(|&module| async move { require_resolve(module.ident().path()).await })
.map(|&source| async move { require_resolve(source.ident().path()).await })
.try_join()
.await?;

Expand Down
35 changes: 33 additions & 2 deletions turbopack/crates/turbopack-resolve/src/ecmascript.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ use turbopack_core::{
issue::{IssueSeverity, IssueSource},
reference_type::{CommonJsReferenceSubType, EcmaScriptModulesReferenceSubType, ReferenceType},
resolve::{
handle_resolve_error,
handle_resolve_error, handle_resolve_source_error,
options::{
ConditionValue, ResolutionConditions, ResolveInPackage, ResolveIntoPackage,
ResolveOptions,
},
origin::{ResolveOrigin, ResolveOriginExt},
parse::Request,
ModuleResolveResult,
resolve, ModuleResolveResult, ResolveResult,
},
};
/// Retrieves the [ResolutionConditions] of both the "into" package (allowing a
Expand Down Expand Up @@ -103,6 +103,37 @@ pub async fn cjs_resolve(
specific_resolve(origin, request, options, ty, issue_severity, issue_source).await
}

#[turbo_tasks::function]
pub async fn cjs_resolve_source(
origin: Vc<Box<dyn ResolveOrigin>>,
request: Vc<Request>,
issue_source: Option<Vc<IssueSource>>,
issue_severity: Vc<IssueSeverity>,
) -> Result<Vc<ResolveResult>> {
// TODO pass CommonJsReferenceSubType
let ty = Value::new(ReferenceType::CommonJs(CommonJsReferenceSubType::Undefined));
let options = apply_cjs_specific_options(origin.resolve_options(ty.clone()))
.resolve()
.await?;
let result = resolve(
origin.origin_path().parent().resolve().await?,
ty.clone(),
request,
options,
);

handle_resolve_source_error(
result,
ty,
origin.origin_path(),
request,
options,
issue_severity,
issue_source,
)
.await
}

async fn specific_resolve(
origin: Vc<Box<dyn ResolveOrigin>>,
request: Vc<Request>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,15 @@ it('should support require.resolve with extensions', () => {
'[project]/turbopack/crates/turbopack-tests/tests/execution/turbopack/resolving/require-resolve/input/resolved.js [test] (ecmascript)'
)
})

it('should support require.resolve on the current module', () => {
expect(require.resolve('./index.js')).toBe(
'[project]/turbopack/crates/turbopack-tests/tests/execution/turbopack/resolving/require-resolve/input/index.js [test] (ecmascript)'
)
// Check if static evaluation is not hanging
if (require.resolve('./index.js') == "") {
throw new Error("no no");
}
})

export {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"treeShakingMode": "reexports-only"
}

0 comments on commit ec7b33e

Please sign in to comment.