Skip to content

Commit 2186b28

Browse files
BoshenBoshen
authored
fix(linter): fix Arc<ModuleRecord> memory leak and lifecycle issues (#14049)
1 parent 10a41ab commit 2186b28

File tree

16 files changed

+108
-76
lines changed

16 files changed

+108
-76
lines changed

‎.ignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ tasks/coverage/test262/**
77
tasks/coverage/babel/**
88
tasks/coverage/typescript/**
99
tasks/prettier_conformance/prettier/**
10+
apps/**/dist
1011

1112
**/*.snap
1213

‎Cargo.lock‎

Lines changed: 9 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/oxc_linter/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ language-tags = { workspace = true }
6060
lazy-regex = { workspace = true }
6161
memchr = { workspace = true }
6262
nonmax = { workspace = true }
63+
papaya = { workspace = true }
6364
phf = { workspace = true, features = ["macros"] }
6465
rayon = { workspace = true }
6566
rust-lapper = { workspace = true }

‎crates/oxc_linter/src/module_graph_visitor.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,16 +189,21 @@ impl ModuleGraphVisitor {
189189
enter: &mut EnterMod,
190190
leave: &mut LeaveMod,
191191
) -> VisitFoldWhile<T> {
192-
for pair in module_record.loaded_modules.read().unwrap().iter() {
192+
for (key, weak_module_record) in module_record.loaded_modules().iter() {
193193
if self.depth > self.max_depth {
194194
return VisitFoldWhile::Stop(accumulator.into_inner());
195195
}
196196

197-
let path = &pair.1.resolved_absolute_path;
197+
let loaded_module_record = weak_module_record.upgrade().unwrap();
198+
199+
let path = &loaded_module_record.resolved_absolute_path;
200+
198201
if !self.traversed.insert(path.clone()) {
199202
continue;
200203
}
201204

205+
let pair = (key, &loaded_module_record);
206+
202207
if !filter(pair, module_record) {
203208
continue;
204209
}

‎crates/oxc_linter/src/module_record.rs‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
use std::{
44
fmt,
55
path::{Path, PathBuf},
6-
sync::{Arc, OnceLock, RwLock},
6+
sync::{Arc, OnceLock, RwLock, RwLockReadGuard, RwLockWriteGuard, Weak},
77
};
88

99
use rustc_hash::FxHashMap;
@@ -46,7 +46,9 @@ pub struct ModuleRecord {
4646
///
4747
/// Note that Oxc does not support cross-file analysis, so this map will be empty after
4848
/// [`ModuleRecord`] is created. You must link the module records yourself.
49-
pub loaded_modules: RwLock<FxHashMap<CompactStr, Arc<ModuleRecord>>>,
49+
///
50+
/// Use [ModuleRecord::get_loaded_module] to get a `ModuleRecord`.
51+
loaded_modules: RwLock<FxHashMap<CompactStr, Weak<ModuleRecord>>>,
5052

5153
/// `[[ImportEntries]]`
5254
///
@@ -508,18 +510,46 @@ impl ModuleRecord {
508510
}
509511
}
510512

513+
/// # Panics
514+
///
515+
/// * If the RwLock is poisoned (which only happens if a thread panicked while holding the lock).
516+
pub fn loaded_modules(&self) -> RwLockReadGuard<'_, FxHashMap<CompactStr, Weak<ModuleRecord>>> {
517+
self.loaded_modules.read().unwrap()
518+
}
519+
520+
/// # Panics
521+
///
522+
/// * If the RwLock is poisoned (which only happens if a thread panicked while holding the lock).
523+
pub fn write_loaded_modules(
524+
&self,
525+
) -> RwLockWriteGuard<'_, FxHashMap<CompactStr, Weak<ModuleRecord>>> {
526+
self.loaded_modules.write().unwrap()
527+
}
528+
529+
/// Get a loaded module by upgrading the weak reference to an Arc.
530+
/// Returns None if the module has been dropped or not found.
531+
///
532+
/// # Panics
533+
///
534+
/// * If the RwLock is poisoned (which only happens if a thread panicked while holding the lock).
535+
/// * If `ModuleRecord` is dropped (fails to Weak::upgrade).
536+
pub fn get_loaded_module(&self, key: &str) -> Option<Arc<ModuleRecord>> {
537+
let loaded_modules = self.loaded_modules();
538+
loaded_modules.get(key).map(|weak| Weak::upgrade(weak).unwrap())
539+
}
540+
511541
pub(crate) fn exported_bindings_from_star_export(
512542
&self,
513543
) -> &FxHashMap<PathBuf, Vec<CompactStr>> {
514544
self.exported_bindings_from_star_export.get_or_init(|| {
515545
let mut exported_bindings_from_star_export: FxHashMap<PathBuf, Vec<CompactStr>> =
516546
FxHashMap::default();
517-
let loaded_modules = self.loaded_modules.read().unwrap();
518547
for export_entry in &self.star_export_entries {
519548
let Some(module_request) = &export_entry.module_request else {
520549
continue;
521550
};
522-
let Some(remote_module_record) = loaded_modules.get(module_request.name()) else {
551+
let Some(remote_module_record) = self.get_loaded_module(module_request.name())
552+
else {
523553
continue;
524554
};
525555
// Append both remote `bindings` and `exported_bindings_from_star_export`

‎crates/oxc_linter/src/rules/import/default.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,13 @@ declare_oxc_lint!(
5454
impl Rule for Default {
5555
fn run_once(&self, ctx: &LintContext<'_>) {
5656
let module_record = ctx.module_record();
57-
let loaded_modules = module_record.loaded_modules.read().unwrap();
5857
for import_entry in &module_record.import_entries {
5958
let ImportImportName::Default(default_span) = import_entry.import_name else {
6059
continue;
6160
};
6261

6362
let specifier = import_entry.module_request.name();
64-
let Some(remote_module_record) = loaded_modules.get(specifier) else {
63+
let Some(remote_module_record) = module_record.get_loaded_module(specifier) else {
6564
continue;
6665
};
6766
if !remote_module_record.has_module_syntax {

‎crates/oxc_linter/src/rules/import/export.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ impl Rule for Export {
5656
let mut all_export_names = FxHashMap::default();
5757
let mut visited = FxHashSet::default();
5858

59-
let loaded_modules = module_record.loaded_modules.read().unwrap();
6059
module_record.star_export_entries.iter().for_each(|star_export_entry| {
6160
if star_export_entry.is_type {
6261
return;
@@ -66,11 +65,12 @@ impl Rule for Export {
6665
let Some(module_request) = &star_export_entry.module_request else {
6766
return;
6867
};
69-
let Some(remote_module_record) = loaded_modules.get(module_request.name()) else {
68+
let Some(remote_module_record) = module_record.get_loaded_module(module_request.name())
69+
else {
7070
return;
7171
};
7272

73-
walk_exported_recursive(remote_module_record, &mut export_names, &mut visited);
73+
walk_exported_recursive(&remote_module_record, &mut export_names, &mut visited);
7474

7575
if export_names.is_empty() {
7676
ctx.diagnostic(no_named_export(module_request.name(), module_request.span));
@@ -118,15 +118,15 @@ fn walk_exported_recursive(
118118
for name in module_record.exported_bindings.keys() {
119119
result.insert(name.clone());
120120
}
121-
let loaded_modules = module_record.loaded_modules.read().unwrap();
122121
for export_entry in &module_record.star_export_entries {
123122
let Some(module_request) = &export_entry.module_request else {
124123
continue;
125124
};
126-
let Some(remote_module_record) = loaded_modules.get(module_request.name()) else {
125+
let Some(remote_module_record) = module_record.get_loaded_module(module_request.name())
126+
else {
127127
continue;
128128
};
129-
walk_exported_recursive(remote_module_record, result, visited);
129+
walk_exported_recursive(&remote_module_record, result, visited);
130130
}
131131
}
132132

‎crates/oxc_linter/src/rules/import/named.rs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,14 @@ impl Rule for Named {
9191

9292
let module_record = ctx.module_record();
9393

94-
let loaded_modules = module_record.loaded_modules.read().unwrap();
9594
for import_entry in &module_record.import_entries {
9695
// Get named import
9796
let ImportImportName::Name(import_name) = &import_entry.import_name else {
9897
continue;
9998
};
10099
let specifier = import_entry.module_request.name();
101100
// Get remote module record
102-
let Some(remote_module_record) = loaded_modules.get(specifier) else {
101+
let Some(remote_module_record) = module_record.get_loaded_module(specifier) else {
103102
continue;
104103
};
105104
if !remote_module_record.has_module_syntax {
@@ -127,7 +126,6 @@ impl Rule for Named {
127126
ctx.diagnostic(named_diagnostic(name, specifier, import_span));
128127
}
129128

130-
let loaded_modules = module_record.loaded_modules.read().unwrap();
131129
for export_entry in &module_record.indirect_export_entries {
132130
let Some(module_request) = &export_entry.module_request else {
133131
continue;
@@ -137,7 +135,7 @@ impl Rule for Named {
137135
};
138136
let specifier = module_request.name();
139137
// Get remote module record
140-
let Some(remote_module_record) = loaded_modules.get(specifier) else {
138+
let Some(remote_module_record) = module_record.get_loaded_module(specifier) else {
141139
continue;
142140
};
143141
if !remote_module_record.has_module_syntax {

‎crates/oxc_linter/src/rules/import/namespace.rs‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -124,32 +124,30 @@ impl Rule for Namespace {
124124
return;
125125
}
126126

127-
let loaded_modules = module_record.loaded_modules.read().unwrap();
128-
129127
for entry in &module_record.import_entries {
130128
let (source, module) = match &entry.import_name {
131129
ImportImportName::NamespaceObject => {
132130
let source = entry.module_request.name();
133-
let Some(module) = loaded_modules.get(source) else {
131+
let Some(module) = module_record.get_loaded_module(source) else {
134132
return;
135133
};
136-
(source.to_string(), Arc::clone(module))
134+
(source.to_string(), module)
137135
}
138136
ImportImportName::Name(name) => {
139-
let Some(loaded_module) = loaded_modules.get(entry.module_request.name())
137+
let Some(loaded_module) =
138+
module_record.get_loaded_module(entry.module_request.name())
140139
else {
141140
return;
142141
};
143-
let Some(source) = get_module_request_name(name.name(), loaded_module) else {
142+
let Some(source) = get_module_request_name(name.name(), &loaded_module) else {
144143
return;
145144
};
146-
147-
let loaded_module = loaded_module.loaded_modules.read().unwrap();
148-
let Some(loaded_module) = loaded_module.get(source.as_str()) else {
145+
let Some(loaded_module_for_source) =
146+
loaded_module.get_loaded_module(source.as_str())
147+
else {
149148
return;
150149
};
151-
152-
(source, Arc::clone(loaded_module))
150+
(source, loaded_module_for_source)
153151
}
154152
ImportImportName::Default(_) => {
155153
// TODO: Hard to confirm if it's a namespace object
@@ -280,11 +278,10 @@ fn check_deep_namespace_for_node(
280278

281279
if let Some(module_source) = get_module_request_name(name, module) {
282280
let parent_node = ctx.nodes().parent_node(node.id());
283-
let loaded_modules = module.loaded_modules.read().unwrap();
284-
let module_record = loaded_modules.get(module_source.as_str())?;
281+
let module_record = module.get_loaded_module(module_source.as_str())?;
285282
let mut namespaces = namespaces.to_owned();
286283
namespaces.push(name.into());
287-
check_deep_namespace_for_node(parent_node, source, &namespaces, module_record, ctx);
284+
check_deep_namespace_for_node(parent_node, source, &namespaces, &module_record, ctx);
288285
} else {
289286
check_binding_exported(
290287
name,
@@ -321,12 +318,11 @@ fn check_deep_namespace_for_object_pattern(
321318
let mut next_namespaces = namespaces.to_owned();
322319
next_namespaces.push(name.to_string());
323320

324-
let loaded_modules = module.loaded_modules.read().unwrap();
325321
check_deep_namespace_for_object_pattern(
326322
pattern,
327323
source,
328324
next_namespaces.as_slice(),
329-
loaded_modules.get(module_source.as_str()).unwrap(),
325+
&module.get_loaded_module(module_source.as_str()).unwrap(),
330326
ctx,
331327
);
332328
continue;

‎crates/oxc_linter/src/rules/import/no_duplicates.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,11 @@ impl Rule for NoDuplicates {
9292
fn run_once(&self, ctx: &LintContext<'_>) {
9393
let module_record = ctx.module_record();
9494

95-
let loaded_modules = module_record.loaded_modules.read().unwrap();
9695
let groups = module_record
9796
.requested_modules
9897
.iter()
9998
.map(|(source, requested_modules)| {
100-
let resolved_absolute_path = loaded_modules.get(source).map_or_else(
99+
let resolved_absolute_path = module_record.get_loaded_module(source).map_or_else(
101100
|| source.to_string(),
102101
|module| module.resolved_absolute_path.to_string_lossy().to_string(),
103102
);

0 commit comments

Comments
 (0)