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

[Shared Resources] Embedded host implementation #261

Merged
merged 38 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
e8032ae
Flesh out the sync Compiler class
jerivas Nov 13, 2023
eb23204
Factor out common compilation helpers
jerivas Nov 13, 2023
9e78051
Add AsyncCompiler
jerivas Nov 13, 2023
4aaa65f
Autofix
jerivas Nov 13, 2023
1caf32e
Bring back legacy checks
jerivas Nov 14, 2023
b41959e
Don't autoclose the sync compiler
jerivas Nov 14, 2023
7906f99
Update module exports
jerivas Nov 14, 2023
ebfddd9
Let active compilations settle before disposing
jerivas Nov 14, 2023
c47a958
update copyright
jgerigmeyer Nov 15, 2023
a6ca5d9
Add try/catch to sync functions as well
jerivas Nov 15, 2023
8ecb0e3
Export compiler classes
jerivas Nov 16, 2023
ba7c156
Remove complete compilations from compiler state
jerivas Nov 16, 2023
cdd32ca
Update comments
jerivas Nov 16, 2023
fbbe407
Add unique compilation IDs
jerivas Nov 16, 2023
c35c965
Lint
jerivas Nov 16, 2023
298daab
Add missing getter
jerivas Nov 16, 2023
06974e1
Reuse package and message transformers
jerivas Nov 21, 2023
7b7fbf7
Throw errors if Compiler classes directly contructed
jamesnw Dec 15, 2023
e7e4868
Merge branch 'main' of https://github.com/sass/embedded-host-node int…
jamesnw Dec 16, 2023
297cf67
Use compiler-level compilation IDs
jerivas Dec 23, 2023
b03ef75
Fix compilation ID reset bug in AsyncCompiler and Compiler
jerivas Dec 27, 2023
ddfb710
Track active compilations when building the request
jerivas Dec 27, 2023
bf7cf3c
Update comments and type annotations
jerivas Jan 4, 2024
7138091
Unsubscribe dispatchers when they execute their callback
jerivas Jan 4, 2024
d79ada4
Use consistent cwd for compiler processes
jerivas Jan 4, 2024
26a0338
Resolve paths before sending them to the compiler process
jerivas Jan 4, 2024
55e64f2
Lint
jerivas Jan 4, 2024
612db59
Add missing await
jerivas Jan 4, 2024
329fa94
Clean up Dispatcher class
jerivas Jan 4, 2024
2556525
Update compiler imports and file structure
jerivas Jan 6, 2024
f5c2e8a
Update copyright year
jerivas Jan 6, 2024
66a7f66
Add comments
jerivas Jan 6, 2024
55a55ed
Update comment
jerivas Jan 8, 2024
0f0b5aa
Merge branch 'main' into feature.shared-resources
jgerigmeyer Jan 8, 2024
2b24ec4
update comments
jgerigmeyer Jan 8, 2024
a222ff2
copyrights are inconsistent in this repo
jgerigmeyer Jan 8, 2024
84ae6a4
Add private id to FunctionRegistry
jerivas Jan 11, 2024
82819b0
Merge branch 'main' into feature.shared-resources
jerivas Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 102 additions & 8 deletions lib/src/async-compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,42 +6,136 @@ import {spawn} from 'child_process';
import {Observable} from 'rxjs';
import {takeUntil} from 'rxjs/operators';

import {
OptionsWithLegacy,
StringOptionsWithLegacy,
createDispatcher,
handleCompileResponse,
handleLogEvent,
newCompilePathRequest,
newCompileStringRequest,
} from './compiler';
import {compilerCommand} from './compiler-path';
import {FunctionRegistry} from './function-registry';
import {ImporterRegistry} from './importer-registry';
import * as utils from './utils';
import * as proto from './vendor/embedded_sass_pb';
import {CompileResult} from './vendor/sass';

/**
* An asynchronous wrapper for the embedded Sass compiler that exposes its stdio
* streams as Observables.
* An asynchronous wrapper for the embedded Sass compiler
*/
export class AsyncEmbeddedCompiler {
export class AsyncCompiler {
/** The underlying process that's being wrapped. */
private readonly process = spawn(
compilerCommand[0],
[...compilerCommand.slice(1), '--embedded'],
{windowsHide: true}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

One more bug. New compilations can crash compiler after cwd of dart-sass process is deleted. This bug actually existed previously, but could not be exploited, because the compiler would be closed before we could remove or change cwd.

Reproduction:

import * as sass from 'sass-embedded';
import * as fs from 'fs';
import { chdir } from 'process';

const main = async function () {
  fs.mkdirSync('test', {recursive: true})
  chdir('test')

  // start compiler in directory test
  const asyncCompiler = await sass.initAsyncCompiler();

  chdir('..')
  fs.rmSync('test', {recursive: true})

  // run compilation after directory test is removed
  fs.writeFileSync('test.scss', 'a { b: c }')
  await asyncCompiler.compileAsync('test.scss')

  await asyncCompiler.dispose();
}

main()

Crash output:

Standalone embedder initialization failed: Error determining current directory: No such file or directory

The cwd of embedded compiler does not affect how imports are resolved, but it can crash dart vm if the cwd is removed. The fix is to change the cwd of dart-sass process to something that can be accessed and is unlikely to be deleted by user (e.g. directory containing the dart-sass binary):

diff --git a/lib/src/async-compiler.ts b/lib/src/async-compiler.ts
index 7e7e00d..1df7703 100644
--- a/lib/src/async-compiler.ts
+++ b/lib/src/async-compiler.ts
@@ -2,6 +2,7 @@
 // MIT-style license that can be found in the LICENSE file or at
 // https://opensource.org/licenses/MIT.
 
+import * as p from 'path';
 import {spawn} from 'child_process';
 import {Observable} from 'rxjs';
 import {takeUntil} from 'rxjs/operators';
@@ -39,7 +40,7 @@ export class AsyncCompiler {
   private readonly process = spawn(
     compilerCommand[0],
     [...compilerCommand.slice(1), '--embedded'],
-    {windowsHide: true}
+    {cwd: p.dirname(compilerCommand[0]), windowsHide: true}
   );
 
   /** The next compilation ID */
diff --git a/lib/src/sync-compiler.ts b/lib/src/sync-compiler.ts
index afe9dfa..f34f600 100644
--- a/lib/src/sync-compiler.ts
+++ b/lib/src/sync-compiler.ts
@@ -2,6 +2,7 @@
 // MIT-style license that can be found in the LICENSE file or at
 // https://opensource.org/licenses/MIT.
 
+import * as p from 'path';
 import {Subject} from 'rxjs';
 
 import {
@@ -39,7 +40,7 @@ export class Compiler {
   private readonly process = new SyncProcess(
     compilerCommand[0],
     [...compilerCommand.slice(1), '--embedded'],
-    {windowsHide: true}
+    {cwd: p.dirname(compilerCommand[0]), windowsHide: true}
   );
 
   /** The next compilation ID */

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like if you delete the CWD out from under a process you're asking for trouble to begin with, but since this looks like an easy fix we might as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d79ada4


/** Whether the underlying compiler has already exited. */
private disposed = false;

/** The child process's exit event. */
readonly exit$ = new Promise<number | null>(resolve => {
private readonly exit$ = new Promise<number | null>(resolve => {
this.process.on('exit', code => resolve(code));
});

/** The buffers emitted by the child process's stdout. */
readonly stdout$ = new Observable<Buffer>(observer => {
private readonly stdout$ = new Observable<Buffer>(observer => {
this.process.stdout.on('data', buffer => observer.next(buffer));
}).pipe(takeUntil(this.exit$));

/** The buffers emitted by the child process's stderr. */
readonly stderr$ = new Observable<Buffer>(observer => {
private readonly stderr$ = new Observable<Buffer>(observer => {
this.process.stderr.on('data', buffer => observer.next(buffer));
}).pipe(takeUntil(this.exit$));

/** Writes `buffer` to the child process's stdin. */
writeStdin(buffer: Buffer): void {
private writeStdin(buffer: Buffer): void {
this.process.stdin.write(buffer);
}

private throwIfDisposed(): void {
jerivas marked this conversation as resolved.
Show resolved Hide resolved
if (this.disposed) {
throw utils.compilerError('Sync compiler has already exited.');
}
}

// Spins up a compiler, then sends it a compile request. Returns a promise that
// resolves with the CompileResult. Throws if there were any protocol or
// compilation errors. Shuts down the compiler after compilation.
private async compileRequestAsync(
request: proto.InboundMessage_CompileRequest,
importers: ImporterRegistry<'async'>,
options?: OptionsWithLegacy<'async'> & {legacy?: boolean}
): Promise<CompileResult> {
const functions = new FunctionRegistry(options?.functions);
this.stderr$.subscribe(data => process.stderr.write(data));

const dispatcher = createDispatcher<'async'>(
this.stdout$,
buffer => {
this.writeStdin(buffer);
},
{
handleImportRequest: request => importers.import(request),
handleFileImportRequest: request => importers.fileImport(request),
handleCanonicalizeRequest: request => importers.canonicalize(request),
handleFunctionCallRequest: request => functions.call(request),
}
);

dispatcher.logEvents$.subscribe(event => handleLogEvent(options, event));

return handleCompileResponse(
await new Promise<proto.OutboundMessage_CompileResponse>(
(resolve, reject) =>
dispatcher.sendCompileRequest(request, (err, response) => {
if (err) {
reject(err);
} else {
resolve(response!);
}
})
)
);
}

compileAsync(
path: string,
options?: OptionsWithLegacy<'async'>
): Promise<CompileResult> {
this.throwIfDisposed();
const importers = new ImporterRegistry(options);
return this.compileRequestAsync(
newCompilePathRequest(path, importers, options),
importers,
options
);
}

compileStringAsync(
source: string,
options?: StringOptionsWithLegacy<'async'>
): Promise<CompileResult> {
this.throwIfDisposed();
const importers = new ImporterRegistry(options);
return this.compileRequestAsync(
newCompileStringRequest(source, importers, options),
importers,
options
);
}

/** Kills the child process, cleaning up all associated Observables. */
close() {
async dispose() {
Copy link
Contributor

Choose a reason for hiding this comment

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

All functions should have declared return types:

Suggested change
async dispose() {
async dispose(): Promise<void> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bf7cf3c

this.disposed = true;
this.process.stdin.end();
await this.exit$;
jamesnw marked this conversation as resolved.
Show resolved Hide resolved
}
}

export async function initAsyncCompiler(): Promise<AsyncCompiler> {
return new AsyncCompiler();
}
Loading