- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 227
Offload Oxide scanning to separate process #1471
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
Conversation
| I can pull it down at work on my windows machine tomorrow to try it out and report back! | 
| So it looks like this isn't possible from my (somewhat limited) testing. Node supposedly handles the unloading of these files but I'm guessing it just doesn't on Windows. If you run this script: let mod = require("./node_modules/@tailwindcss/oxide-win32-arm64-msvc/tailwindcss-oxide.win32-arm64-msvc.node")
console.log(mod);
mod = null
delete require.cache["./node_modules/@tailwindcss/oxide-win32-arm64-msvc/tailwindcss-oxide.win32-arm64-msvc.node"];
setTimeout(() => {}, 60_000)and then try to run  | 
b8416c7    to
    21c7dca      
    Compare
  
    00a0a62    to
    cb0a073      
    Compare
  
    | @DustinsCode If you wanna give this PR a go it should work (does in my testing) | 
c655125    to
    e730887      
    Compare
  
    | // TODO: Can we find a way to not require a build first? | ||
| // let module = path.resolve(path.dirname(__filename), './oxide-helper.ts') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm leaving this in as a note for me to look into later. Right now editing the helper file and re-running tests requires a rebuild. But this is "fine" for now.
CI already does this (b/c a few tests have to actually test against the built version for reasons).
These are expected if you are just using the plain `tailwindcss` package or don’t have `tailwindcss` installed locally at all — for example when using the Standalone CLI. Other errors should be surfaced though as they could indicate a problem.
e730887    to
    c537a5b      
    Compare
  
    
Loading Oxide's .node file into the language server process / VSCode extension host on Windows marks that .node file as in use. When this happens you cannot completely delete node_modules (for example by running
npm ci).To work around this we'll fork a new process that can load Oxide, run its scan(s), and then exit. During initial project discovery we use a temporarily long lived process that persists for the duration of project discovery — which may include multiple Oxide scans across projects (and even versions). Once the project discovery has completed the process will exit. For any subsequent content scanning (e.g. when CSS changes) we'll spawn a new, temporary process for that individual scan.
This will ensure that, once the process has exited, the
.nodefile is no longer considered to be in-use and commands likenpm ciwill run properly.Commentary
Why not use
require.cache?Unfortunately, deleting entries from
require.cachedoes not unload the Oxide binary from the process address space.Why not worker threads
So, this might work but also might not. You'd still be loading it into the processes address space so there's a chance that as long as the process is open — whether the thread has exited or not — the
.nodefile would still be marked as in use.Additionally, we have some flags set when building Oxide that basically prevent it from unloading in worker threads due to some bugs in the Rust standard library. This applies to Linux only iirc so it shouldn't actually be a problem there but I'd rather keep the mechanism working consistently across operating systems.
Communication between processes
The main process and helper communicate using a JSON-RPC protocol — similar to the one used by language servers/clients — but without any initialization setup. This is an internal tool and the message format is not considered stable and may change in any future version.
Communication happens over an IPC channel provided by
child_process.fork(…). As far as I am aware, this uses private file descriptors shared between processes. No other process should be capable of "tricking" the helper into loading other.nodefiles into its address space. Only the ones we discover during NPM package resolution should ever be loaded. Even though the temporary helper isn't active for very long this was still a concern I had while developing this.Test Plan
There are automated tests that verify existing functionality still works but testing this specific scenario on Windows in an automated fashion with the current test setup would be a bit annoying so I did some additional manual testing:
npm ciin the terminal and watched it fail b/c of the Oxide.nodefilenpm cimultiple times to make sure it worked