From e191b665cc12799ace3cf56fe27e0f780948d8e7 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Tue, 14 Feb 2023 11:10:12 -0800 Subject: [PATCH] Restore Issue Reporters (vercel/turbo#3803) This restores issue reporters, addressing a bug that prevented turbo-trace from completing. This moves `ConsoleUiVc::new` into an async block to prevent it from stalling. Test Plan: Verify `cargo run --bin node-file-trace -- print path/to/my/app` no longer stalls. --- crates/next-dev/src/lib.rs | 85 +++++++++++++------ crates/node-file-trace/src/lib.rs | 28 +++--- crates/turbopack-cli-utils/src/issue.rs | 61 ++++++------- crates/turbopack-core/src/issue/mod.rs | 27 +++++- crates/turbopack-dev-server/Cargo.toml | 1 + crates/turbopack-dev-server/src/http.rs | 11 ++- crates/turbopack-dev-server/src/lib.rs | 45 +++++----- .../src/source/resolve.rs | 6 +- .../turbopack-dev-server/src/update/server.rs | 11 ++- 9 files changed, 168 insertions(+), 107 deletions(-) diff --git a/crates/next-dev/src/lib.rs b/crates/next-dev/src/lib.rs index c8efb922eca65..7095dc54da096 100644 --- a/crates/next-dev/src/lib.rs +++ b/crates/next-dev/src/lib.rs @@ -27,14 +27,15 @@ use owo_colors::OwoColorize; use turbo_malloc::TurboMalloc; use turbo_tasks::{ util::{FormatBytes, FormatDuration}, - RawVc, StatsType, TransientInstance, TransientValue, TurboTasks, TurboTasksBackendApi, Value, + CollectiblesSource, RawVc, StatsType, TransientInstance, TransientValue, TurboTasks, + TurboTasksBackendApi, Value, }; use turbo_tasks_fs::{DiskFileSystemVc, FileSystem, FileSystemVc}; use turbo_tasks_memory::MemoryBackend; -use turbopack_cli_utils::issue::{ConsoleUi, ConsoleUiVc, LogOptions}; +use turbopack_cli_utils::issue::{ConsoleUiVc, LogOptions}; use turbopack_core::{ environment::ServerAddr, - issue::IssueSeverity, + issue::{IssueReporter, IssueReporterVc, IssueSeverity, IssueVc}, resolve::{parse::RequestVc, pattern::QueryMapVc}, server_fs::ServerFileSystemVc, }; @@ -62,6 +63,7 @@ pub struct NextDevServerBuilder { entry_requests: Vec, eager_compile: bool, hostname: Option, + issue_reporter: Option>, port: Option, browserslist_query: String, log_level: IssueSeverity, @@ -83,6 +85,7 @@ impl NextDevServerBuilder { entry_requests: vec![], eager_compile: false, hostname: None, + issue_reporter: None, port: None, browserslist_query: "last 1 Chrome versions, last 1 Firefox versions, last 1 Safari \ versions, last 1 Edge versions" @@ -139,6 +142,14 @@ impl NextDevServerBuilder { self } + pub fn issue_reporter( + mut self, + issue_reporter: Box, + ) -> NextDevServerBuilder { + self.issue_reporter = Some(issue_reporter); + self + } + /// Attempts to find an open port to bind. fn find_port(&self, host: IpAddr, port: u16, max_attempts: u16) -> Result { // max_attempts of 1 means we loop 0 times. @@ -192,17 +203,22 @@ impl NextDevServerBuilder { let show_all = self.show_all; let log_detail = self.log_detail; let browserslist_query = self.browserslist_query; - let log_options = LogOptions { + let log_options = Arc::new(LogOptions { current_dir: current_dir().unwrap(), show_all, log_detail, log_level: self.log_level, - }; + }); let entry_requests = Arc::new(self.entry_requests); - let console_ui = Arc::new(ConsoleUi::new(log_options)); - let console_ui_to_dev_server = console_ui.clone(); let server_addr = Arc::new(server.addr); let tasks = turbo_tasks.clone(); + let issue_provider = self.issue_reporter.unwrap_or_else(|| { + // Initialize a ConsoleUi reporter if no custom reporter was provided + Box::new(move || ConsoleUiVc::new(log_options.clone().into()).into()) + }); + let issue_reporter_arc = Arc::new(move || issue_provider.get_issue_reporter()); + + let get_issue_reporter = issue_reporter_arc.clone(); let source = move || { source( root_dir.clone(), @@ -210,22 +226,31 @@ impl NextDevServerBuilder { entry_requests.clone().into(), eager_compile, turbo_tasks.clone().into(), - console_ui.clone().into(), + get_issue_reporter(), browserslist_query.clone(), server_addr.clone().into(), ) }; - Ok(server.serve(tasks, source, console_ui_to_dev_server)) + Ok(server.serve(tasks, source, issue_reporter_arc.clone())) } } -async fn handle_issues>(source: T, console_ui: ConsoleUiVc) -> Result<()> { - let state = console_ui - .group_and_display_issues(TransientValue::new(source.into())) +async fn handle_issues + CollectiblesSource + Copy>( + source: T, + issue_reporter: IssueReporterVc, +) -> Result<()> { + let issues = IssueVc::peek_issues_with_path(source) + .await? + .strongly_consistent() .await?; - if state.has_fatal { + issue_reporter.report_issues( + TransientInstance::new(issues.clone()), + TransientValue::new(source.into()), + ); + + if issues.has_fatal().await? { Err(anyhow!("Fatal issue(s) occurred")) } else { Ok(()) @@ -233,17 +258,17 @@ async fn handle_issues>(source: T, console_ui: ConsoleUiVc) -> Re } #[turbo_tasks::function] -async fn project_fs(project_dir: &str, console_ui: ConsoleUiVc) -> Result { +async fn project_fs(project_dir: &str, issue_reporter: IssueReporterVc) -> Result { let disk_fs = DiskFileSystemVc::new("project".to_string(), project_dir.to_string()); - handle_issues(disk_fs, console_ui).await?; + handle_issues(disk_fs, issue_reporter).await?; disk_fs.await?.start_watching()?; Ok(disk_fs.into()) } #[turbo_tasks::function] -async fn output_fs(project_dir: &str, console_ui: ConsoleUiVc) -> Result { +async fn output_fs(project_dir: &str, issue_reporter: IssueReporterVc) -> Result { let disk_fs = DiskFileSystemVc::new("output".to_string(), project_dir.to_string()); - handle_issues(disk_fs, console_ui).await?; + handle_issues(disk_fs, issue_reporter).await?; disk_fs.await?.start_watching()?; Ok(disk_fs.into()) } @@ -256,13 +281,12 @@ async fn source( entry_requests: TransientInstance>, eager_compile: bool, turbo_tasks: TransientInstance>, - console_ui: TransientInstance, + issue_reporter: IssueReporterVc, browserslist_query: String, server_addr: TransientInstance, ) -> Result { - let console_ui = (*console_ui).clone().cell(); - let output_fs = output_fs(&project_dir, console_ui); - let fs = project_fs(&root_dir, console_ui); + let output_fs = output_fs(&project_dir, issue_reporter); + let fs = project_fs(&root_dir, issue_reporter); let project_relative = project_dir.strip_prefix(&root_dir).unwrap(); let project_relative = project_relative .strip_prefix(MAIN_SEPARATOR) @@ -372,9 +396,9 @@ async fn source( .cell() .into(); - handle_issues(dev_server_fs, console_ui).await?; - handle_issues(web_source, console_ui).await?; - handle_issues(page_source, console_ui).await?; + handle_issues(dev_server_fs, issue_reporter).await?; + handle_issues(web_source, issue_reporter).await?; + handle_issues(page_source, issue_reporter).await?; Ok(source) } @@ -551,3 +575,16 @@ fn profile_timeout( ) -> impl Future { future } + +pub trait IssueReporterProvider: Send + Sync + 'static { + fn get_issue_reporter(&self) -> IssueReporterVc; +} + +impl IssueReporterProvider for T +where + T: Fn() -> IssueReporterVc + Send + Sync + Clone + 'static, +{ + fn get_issue_reporter(&self) -> IssueReporterVc { + self() + } +} diff --git a/crates/node-file-trace/src/lib.rs b/crates/node-file-trace/src/lib.rs index 1dcf76412009c..42697f84de714 100644 --- a/crates/node-file-trace/src/lib.rs +++ b/crates/node-file-trace/src/lib.rs @@ -37,12 +37,12 @@ use turbopack::{ resolve_options_context::ResolveOptionsContext, transition::TransitionsByNameVc, ModuleAssetContextVc, }; -use turbopack_cli_utils::issue::{ConsoleUi, IssueSeverityCliOption, LogOptions}; +use turbopack_cli_utils::issue::{ConsoleUiVc, IssueSeverityCliOption, LogOptions}; use turbopack_core::{ asset::{Asset, AssetVc, AssetsVc}, context::{AssetContext, AssetContextVc}, environment::{EnvironmentIntention, EnvironmentVc, ExecutionEnvironment, NodeJsEnvironment}, - issue::{IssueSeverity, IssueVc}, + issue::{IssueReporter, IssueSeverity, IssueVc}, reference::all_assets, resolve::options::{ImportMapping, ResolvedMap}, source_asset::SourceAssetVc, @@ -487,25 +487,29 @@ async fn run>( let (sender, mut receiver) = channel(1); let dir = current_dir().unwrap(); let tt = create_tt(); - let console_ui = Arc::new(ConsoleUi::new(LogOptions { - current_dir: dir.clone(), - show_all, - log_detail, - log_level: log_level.map_or_else(|| IssueSeverity::Error, |l| l.0), - })); let task = tt.spawn_root_task(move || { let dir = dir.clone(); let args = args.clone(); - let console_ui = console_ui.clone(); let sender = sender.clone(); Box::pin(async move { let output = main_operation(TransientValue::new(dir.clone()), args.clone().into()); - let console_ui = (*console_ui).clone().cell(); - console_ui - .group_and_display_issues(TransientValue::new(output.into())) + let source = TransientValue::new(output.into()); + let issues = IssueVc::peek_issues_with_path(output) + .await? + .strongly_consistent() .await?; + let console_ui = ConsoleUiVc::new(TransientInstance::new(LogOptions { + current_dir: dir.clone(), + show_all, + log_detail, + log_level: log_level.map_or_else(|| IssueSeverity::Error, |l| l.0), + })); + console_ui + .as_issue_reporter() + .report_issues(TransientInstance::new(issues), source); + if has_return_value { let output_read_ref = output.await?; let output_iter = output_read_ref.iter().cloned(); diff --git a/crates/turbopack-cli-utils/src/issue.rs b/crates/turbopack-cli-utils/src/issue.rs index 7f9303dc2df73..52f8a6bd4f310 100644 --- a/crates/turbopack-cli-utils/src/issue.rs +++ b/crates/turbopack-cli-utils/src/issue.rs @@ -10,15 +10,17 @@ use std::{ use anyhow::{anyhow, Result}; use crossterm::style::{StyledContent, Stylize}; use owo_colors::{OwoColorize as _, Style}; -use turbo_tasks::{RawVc, TransientValue, TryJoinIterExt, ValueToString}; +use turbo_tasks::{ + RawVc, ReadRef, TransientInstance, TransientValue, TryJoinIterExt, ValueToString, +}; use turbo_tasks_fs::{ attach::AttachedFileSystemVc, source_context::{get_source_context, SourceContextLine}, to_sys_path, FileLinesContent, FileSystemPathVc, }; use turbopack_core::issue::{ - Issue, IssueProcessingPathItem, IssueSeverity, IssueVc, OptionIssueProcessingPathItemsVc, - PlainIssue, PlainIssueSource, + CapturedIssues, Issue, IssueProcessingPathItem, IssueReporter, IssueReporterVc, IssueSeverity, + OptionIssueProcessingPathItemsVc, PlainIssue, PlainIssueSource, }; #[derive(Clone, Copy, PartialEq, Eq, Debug)] @@ -412,41 +414,34 @@ impl PartialEq for ConsoleUi { } } -impl ConsoleUi { - pub fn new(options: LogOptions) -> Self { +#[turbo_tasks::value_impl] +impl ConsoleUiVc { + #[turbo_tasks::function] + pub fn new(options: TransientInstance) -> Self { ConsoleUi { - options, + options: (*options).clone(), seen: Arc::new(Mutex::new(SeenIssues::new())), } + .cell() } } -#[turbo_tasks::value(transparent)] -pub struct DisplayIssueState { - pub has_fatal: bool, - pub has_issues: bool, - pub has_new_issues: bool, -} - #[turbo_tasks::value_impl] -impl ConsoleUiVc { +impl IssueReporter for ConsoleUi { #[turbo_tasks::function] - pub async fn group_and_display_issues( - self, + async fn report_issues( + &self, + issues: TransientInstance>, source: TransientValue, - ) -> Result { - let source = source.into_value(); - let this = self.await?; - - let issues = IssueVc::peek_issues_with_path(source).await?; - let issues = issues.await?; - let &LogOptions { + ) -> Result<()> { + let issues = &*issues; + let LogOptions { ref current_dir, show_all, log_detail, log_level, .. - } = &this.options; + } = self.options; let mut grouped_issues: GroupedIssues = HashMap::new(); let issues = issues @@ -464,11 +459,11 @@ impl ConsoleUiVc { .iter() .map(|(_, _, _, id)| *id) .collect::>(); - let mut new_ids = this.seen.lock().unwrap().new_ids(source, issue_ids); - - let mut has_fatal = false; - let has_issues = !issues.is_empty(); - let has_new_issues = !new_ids.is_empty(); + let mut new_ids = self + .seen + .lock() + .unwrap() + .new_ids(source.into_value(), issue_ids); for (plain_issue, path, context, id) in issues { if !new_ids.remove(&id) { @@ -479,7 +474,6 @@ impl ConsoleUiVc { let context_path = make_relative_to_cwd(context, current_dir).await?; let category = &plain_issue.category; let title = &plain_issue.title; - has_fatal = severity == IssueSeverity::Fatal; let severity_map = grouped_issues .entry(severity) .or_insert_with(Default::default); @@ -612,12 +606,7 @@ impl ConsoleUiVc { } } - Ok(DisplayIssueState { - has_fatal, - has_issues, - has_new_issues, - } - .cell()) + Ok(()) } } diff --git a/crates/turbopack-core/src/issue/mod.rs b/crates/turbopack-core/src/issue/mod.rs index 6af1c8407171c..1ab17a3d38046 100644 --- a/crates/turbopack-core/src/issue/mod.rs +++ b/crates/turbopack-core/src/issue/mod.rs @@ -17,7 +17,8 @@ use futures::FutureExt; use turbo_tasks::{ emit, primitives::{BoolVc, StringVc, U64Vc}, - CollectiblesSource, ReadRef, TryJoinIterExt, ValueToString, ValueToStringVc, + CollectiblesSource, RawVc, ReadRef, TransientInstance, TransientValue, TryJoinIterExt, + ValueToString, ValueToStringVc, }; use turbo_tasks_fs::{ FileContent, FileContentReadRef, FileLine, FileLinesContent, FileSystemPathReadRef, @@ -340,6 +341,21 @@ pub struct CapturedIssues { processing_path: ItemIssueProcessingPathVc, } +impl CapturedIssues { + pub async fn has_fatal(&self) -> Result { + let mut has_fatal = false; + + for issue in self.issues.iter() { + let severity = *issue.severity().await?; + if severity == IssueSeverity::Fatal { + has_fatal = true; + break; + } + } + Ok(has_fatal) + } +} + #[turbo_tasks::value_impl] impl CapturedIssuesVc { #[turbo_tasks::function] @@ -568,3 +584,12 @@ impl PlainAssetVc { .cell()) } } + +#[turbo_tasks::value_trait] +pub trait IssueReporter { + fn report_issues( + &self, + issues: TransientInstance>, + source: TransientValue, + ); +} diff --git a/crates/turbopack-dev-server/Cargo.toml b/crates/turbopack-dev-server/Cargo.toml index ab19c1e9623c7..28959cea404ea 100644 --- a/crates/turbopack-dev-server/Cargo.toml +++ b/crates/turbopack-dev-server/Cargo.toml @@ -17,6 +17,7 @@ hyper-tungstenite = "0.8.1" indexmap = { workspace = true, features = ["serde"] } mime = "0.3.16" mime_guess = "2.0.4" +once_cell = "1.13.0" parking_lot = "0.12.1" pin-project-lite = "0.2.9" serde = "1.0.136" diff --git a/crates/turbopack-dev-server/src/http.rs b/crates/turbopack-dev-server/src/http.rs index 695e40d050fae..91b1851d28cd3 100644 --- a/crates/turbopack-dev-server/src/http.rs +++ b/crates/turbopack-dev-server/src/http.rs @@ -4,8 +4,7 @@ use hyper::{header::HeaderName, Request, Response}; use mime_guess::mime; use turbo_tasks::TransientInstance; use turbo_tasks_fs::{FileContent, FileContentReadRef}; -use turbopack_cli_utils::issue::ConsoleUiVc; -use turbopack_core::{asset::AssetContent, version::VersionedContent}; +use turbopack_core::{asset::AssetContent, issue::IssueReporterVc, version::VersionedContent}; use crate::source::{ request::SourceRequest, @@ -30,10 +29,10 @@ enum GetFromSourceResult { async fn get_from_source( source: ContentSourceVc, request: TransientInstance, - console_ui: ConsoleUiVc, + issue_repoter: IssueReporterVc, ) -> Result { Ok( - match &*resolve_source_request(source, request, console_ui).await? { + match &*resolve_source_request(source, request, issue_repoter).await? { ResolveSourceRequestResult::Static(static_content_vc) => { let static_content = static_content_vc.await?; if let AssetContent::File(file) = &*static_content.content.content().await? { @@ -60,11 +59,11 @@ async fn get_from_source( pub async fn process_request_with_content_source( source: ContentSourceVc, request: Request, - console_ui: ConsoleUiVc, + issue_reporter: IssueReporterVc, ) -> Result> { let original_path = request.uri().path().to_string(); let request = http_request_to_source_request(request).await?; - let result = get_from_source(source, TransientInstance::new(request), console_ui); + let result = get_from_source(source, TransientInstance::new(request), issue_reporter); match &*result.strongly_consistent().await? { GetFromSourceResult::Static { content, diff --git a/crates/turbopack-dev-server/src/lib.rs b/crates/turbopack-dev-server/src/lib.rs index edf9a4d1c0c07..9851b65ff5682 100644 --- a/crates/turbopack-dev-server/src/lib.rs +++ b/crates/turbopack-dev-server/src/lib.rs @@ -16,16 +16,17 @@ use std::{ time::{Duration, Instant}, }; -use anyhow::{bail, Context, Result}; +use anyhow::{anyhow, Context, Result}; use hyper::{ server::{conn::AddrIncoming, Builder}, service::{make_service_fn, service_fn}, Request, Response, Server, }; use turbo_tasks::{ - run_once, trace::TraceRawVcs, util::FormatDuration, RawVc, TransientValue, TurboTasksApi, + run_once, trace::TraceRawVcs, util::FormatDuration, CollectiblesSource, RawVc, + TransientInstance, TransientValue, TurboTasksApi, }; -use turbopack_cli_utils::issue::{ConsoleUi, ConsoleUiVc}; +use turbopack_core::issue::{IssueReporter, IssueReporterVc, IssueVc}; use self::{ source::{ContentSourceResultVc, ContentSourceVc}, @@ -66,21 +67,27 @@ pub struct DevServer { pub future: Pin> + Send + 'static>>, } -// Just print issues to console for now... -async fn handle_issues>( +async fn handle_issues + CollectiblesSource + Copy>( source: T, path: &str, operation: &str, - console_ui: ConsoleUiVc, + issue_reporter: IssueReporterVc, ) -> Result<()> { - let state = console_ui - .group_and_display_issues(TransientValue::new(source.into())) + let issues = IssueVc::peek_issues_with_path(source) + .await? + .strongly_consistent() .await?; - if state.has_fatal { - bail!("Fatal issue(s) occurred in {path} ({operation}") - } - Ok(()) + issue_reporter.report_issues( + TransientInstance::new(issues.clone()), + TransientValue::new(source.into()), + ); + + if issues.has_fatal().await? { + Err(anyhow!("Fatal issue(s) occurred in {path} ({operation})")) + } else { + Ok(()) + } } impl DevServer { @@ -106,21 +113,21 @@ impl DevServerBuilder { self, turbo_tasks: Arc, source_provider: impl SourceProvider + Clone + Send + Sync, - console_ui: Arc, + get_issue_reporter: Arc IssueReporterVc + Send + Sync>, ) -> DevServer { let make_svc = make_service_fn(move |_| { let tt = turbo_tasks.clone(); let source_provider = source_provider.clone(); - let console_ui = console_ui.clone(); + let get_issue_reporter = get_issue_reporter.clone(); async move { let handler = move |request: Request| { - let console_ui = console_ui.clone(); let start = Instant::now(); let tt = tt.clone(); + let get_issue_reporter = get_issue_reporter.clone(); let source_provider = source_provider.clone(); let future = async move { run_once(tt.clone(), async move { - let console_ui = (*console_ui).clone().cell(); + let issue_reporter = get_issue_reporter(); if hyper_tungstenite::is_upgrade_request(&request) { let uri = request.uri(); @@ -130,7 +137,7 @@ impl DevServerBuilder { let (response, websocket) = hyper_tungstenite::upgrade(request, None)?; let update_server = - UpdateServer::new(source_provider, console_ui); + UpdateServer::new(source_provider, issue_reporter); update_server.run(&*tt, websocket); return Ok(response); } @@ -158,12 +165,12 @@ impl DevServerBuilder { let uri = request.uri(); let path = uri.path().to_string(); let source = source_provider.get_source(); - handle_issues(source, &path, "get source", console_ui).await?; + handle_issues(source, &path, "get source", issue_reporter).await?; let resolved_source = source.resolve_strongly_consistent().await?; let response = http::process_request_with_content_source( resolved_source, request, - console_ui, + issue_reporter, ) .await?; let status = response.status().as_u16(); diff --git a/crates/turbopack-dev-server/src/source/resolve.rs b/crates/turbopack-dev-server/src/source/resolve.rs index aa5cd38ab5303..96e827842382a 100644 --- a/crates/turbopack-dev-server/src/source/resolve.rs +++ b/crates/turbopack-dev-server/src/source/resolve.rs @@ -6,7 +6,7 @@ use std::{ use anyhow::{bail, Result}; use hyper::Uri; use turbo_tasks::{TransientInstance, Value}; -use turbopack_cli_utils::issue::ConsoleUiVc; +use turbopack_core::issue::IssueReporterVc; use super::{ headers::{HeaderValue, Headers}, @@ -36,7 +36,7 @@ pub enum ResolveSourceRequestResult { pub async fn resolve_source_request( source: ContentSourceVc, request: TransientInstance, - console_ui: ConsoleUiVc, + issue_reporter: IssueReporterVc, ) -> Result { let mut data = ContentSourceData::default(); let mut current_source = source; @@ -50,7 +50,7 @@ pub async fn resolve_source_request( result, &original_path, "get content from source", - console_ui, + issue_reporter, ) .await?; diff --git a/crates/turbopack-dev-server/src/update/server.rs b/crates/turbopack-dev-server/src/update/server.rs index 2ff147f7fca5c..d4719ac194199 100644 --- a/crates/turbopack-dev-server/src/update/server.rs +++ b/crates/turbopack-dev-server/src/update/server.rs @@ -12,8 +12,7 @@ use tokio::select; use tokio_stream::StreamMap; use turbo_tasks::{TransientInstance, TurboTasksApi}; use turbo_tasks_fs::json::parse_json_with_source_context; -use turbopack_cli_utils::issue::ConsoleUiVc; -use turbopack_core::version::Update; +use turbopack_core::{issue::IssueReporterVc, version::Update}; use super::{ protocol::{ClientMessage, ClientUpdateInstruction, Issue, ResourceIdentifier}, @@ -28,15 +27,15 @@ use crate::{ /// A server that listens for updates and sends them to connected clients. pub(crate) struct UpdateServer { source_provider: P, - console_ui: ConsoleUiVc, + issue_reporter: IssueReporterVc, } impl UpdateServer

{ /// Create a new update server with the given websocket and content source. - pub fn new(source_provider: P, console_ui: ConsoleUiVc) -> Self { + pub fn new(source_provider: P, issue_reporter: IssueReporterVc) -> Self { Self { source_provider, - console_ui, + issue_reporter, } } @@ -69,7 +68,7 @@ impl UpdateServer

{ resolve_source_request( source, TransientInstance::new(request), - self.console_ui + self.issue_reporter ) } };