Render with custom import hangs

[xzyfer]

Given the following fs.readFile never executes it's callback and process hang indefinitely.

// entry.scss
@import "_partial";

// _partial.scss
foo{}

var fs = require('fs');
var sass = require('node-sass');

var files = [
    'entry.scss',
    'entry.scss',
    'entry.scss',
    'entry.scss',
];

files.forEach(function (file) {
    sass.render({
        file: file,
        importer: function (uri, prev, done) {
            var filename = process.cwd() + '/' + uri;
            console.log('fs.readFile:', filename);
            fs.readFile(filename, function(err, data) {
                console.log('I am never executed');
                done({ contents: data });
            });
        }
    }, function(err, result) {
        console.log('successfully rendered');
    });
});

Outputs this and hangs

➜  node-sass-test  node index.js
fs.readFile: /Users/michael/node-sass-test/_partial.scss
fs.readFile: /Users/michael/node-sass-test/_partial.scss
fs.readFile: /Users/michael/node-sass-test/_partial.scss
fs.readFile: /Users/michael/node-sass-test/_partial.scss

This example works fine if:

• I only have 3 files
• I use renderSync
• I use fs.readFileSync

This is potentially related to #794.

---

This issue has been reproduced on multiple OSX machines on the latest node 0.10, 0.12 and iojs with [email protected]

[xzyfer]

/cc @saper (you seem to know your way around this area) 😄

[saper]

I think I have it identified. Workaround: env UV_THREADPOOL_SIZE=5 node indes.js :)

[xzyfer]

Nice, I'll dig into it. I've just started reading the node addon docs :)

[xzyfer]

Looks like that's the legit solution haha nice one.

Ideally we could catch when the queue was full and handle it more gracefully.

[saper]

I have a fix in my head just need to go via the usual write-compile-test-repeat cycle :)
Good news maybe we can even simplify code this time

[xzyfer]

I'd love to see it. I've been reading over the relevant docs but I can't a way to increase the thread pool during execution or stop the queue from being flooded.

[xzyfer]

This seems to suggest it can be done in runtime if it's early enough

joyent/libuv#649 (comment)
mapbox/mapbox-studio-classic#16 (comment)

[saper]

this is a bug. it just explains why 4 (the default) seems to the magic number.

[xzyfer]

I've seen suggestions that this does not work reliably on Windows. It will require some further investigation but atm it blocks custom importers with render

[saper]

Given the state of things I consider going back to 2.1.1 code base.... There are some inherent issues in the callback bridge that do not seem easy to solve (for example we call v8 functions and alter its state in new threads).

just tried 2.1.1 - it hangs the same way as well, because it exhausts uv thread pool the same way.

[saper]

I currently see two solutions:

• Long-term: custom threadpools implemented Pluggable thread pool libuv/libuv#267
• Implement a single thread talking to libsass C API instead of creating a new worked for every render() invocation. One of the implementation ideas I currently have is based on Serializable libsass context? libsass#1108

[mgreter]

Wouldn't it be easier to just queue the work load? Wouldn't any other implementation need a queue too at some point? IMO you will run out of cpus and other resources anyway at some point. I really have only an overview of what you are doing in node-sass by glancing over the issues from time to time. But given the problem as far as I understand it, I would try that approach first. If I'm correct you already have a limited amount of threads available to dispatch work to, so this model should fit very well?

[saper]

We are queuing the rendering requests nicely in the thread pool maintained by the libuv for us. We will take only as many threads as we have available (other workers will just wait).

Here's what happens as I understand it:

1. we queue rendering requests in the libuv threadpool, so free threads will be used to pull our requests out of the queue. This is very nice feaure of libuv, and we can run libsass renderers in parallel.
2. A custom importer gets called by the function provided via sass_make_importer()
3. This thread need to stop and wait until asynchronous user importer in JavaScript completes, because we need to return some value back to the libsass library.
4. So if we quickly create 5 renderes we have 1 renderer waiting in the queue and 4 threads waiting for async user importer to deliver the code to be imported.
5. User code uses asynchronous event-based filesystem API - it does not block execution but filesystem API needs worker threads from the thread pool to deliver the results (that's what makes node so fast and lock-free).
6. Because we share the pool - we got deadlocked, because all 4 active threads are waiting, and the filesystem code ends up waiting in the queue.

Solutions:

A) It would be cool to be able to tell libsass "Don't hold your breath, I'll come back when I am ready" (Hollywood Principle). Frankly, I wouldn't expect such an interface from a simple C library except from some high performance ones. (I would need to be able to come back to sass_compile_file_context and say "Please continue from here, here are your new imports."). Not impossible, but a serious change to libsass operation. Let's call this API sass_compile_continue(Sass_Context *, Sass_Compiler *, return_value);

B) I could use libcoro or some other clever hack to postpone execution of the libsass rendering thread - move aside current the C stack and quit my worker thread. When the user importer is read, a callback handler would restore libsass C stack and pretend nothing happened (and use return(return_value) instead of sass_compile_continue() API described above. No cooperation from libsass is needed.

C) A rendering worker thread, instead of sleeping and waiting for JavaScript to continue it could yield its time slices to execute other code - node has a process.nexttick facility for that. Unfortunately, since node is (almost) single-threaded, I cannot wake up node's execution queue from another, non-node thread.

D) Another solution would be to have our own private thread pool (that's libuv/libuv#267), not to conflict with filesystem thread pool.

E) Create our own one worker thread that communicates two-way with the user custom function/importing code on one side and libsass on the other (what I drafted in sass/libsass#1108). We can use a pipe or sockets to communicate without using locks. This is a variant of (B), but implemented with pipes and not C library calls and does not require me to change libsass at all; I only would love to be able to send (transparently) some libsass internal structures (like Sass_Context *) over sockets.

[xzyfer]

Moving this to v3.next because

• this feature is marked as experimental
• significant work is required
• getting Libsass 3.2.0 to users sooner is the bigger win

[saper]

Yes, #857 and probably #894 can be solved by rewriting a bit of our binding code.

[jhnns]

To be honest, I haven't understand the problem completely. 😀

But would it be possible to add a quickfix for this in sass-loader? Like limiting I/O operations to one at a time?

[saper]

No, there is no quick fix. unless when you pre-load your files or something like that. Or sync.

[xzyfer]

@jhnns there's not quick fix we can implement. However you can set the follow the UV_THREADPOOL_SIZE env variable to an int < 128. The default is 4.

Some projects max this out on load (mapbox/mapbox-studio-classic@c9cda61). This will technically work around the problem, but it'll allow the node process to consume a lot of resources

[wmertens]

Is there any progress on this, or otherwise a way to show a message that there is a deadlock?

[xzyfer]

otherwise a way to show a message that there is a deadlock

Unfortunately a dead lock, prevents us doing anything.

[wmertens]

Not even starting a watchdog thread before the other threads, which warns
when the conversion takes too long?

On Wed, Jan 20, 2016, 11:05 AM Michael Mifsud [email protected]
wrote:

otherwise a way to show a message that there is a deadlock

Unfortunately a dead lock, prevents us doing anything.

—
Reply to this email directly or view it on GitHub
#857 (comment).

Wout.
(typed on mobile, excuse terseness)

[xzyfer]

PRs welcome
On Jan 21, 2016 12:05 AM, "Wout Mertens" [email protected] wrote:

Not even starting a watchdog thread before the other threads, which warns
when the conversion takes too long?

On Wed, Jan 20, 2016, 11:05 AM Michael Mifsud [email protected]
wrote:

otherwise a way to show a message that there is a deadlock

Unfortunately a dead lock, prevents us doing anything.

—
Reply to this email directly or view it on GitHub
#857 (comment).

Wout.
(typed on mobile, excuse terseness)

—
Reply to this email directly or view it on GitHub
#857 (comment).

[saper]

Folk affected by this bug are free to try #1112 (it probably needs to be rebased on top of the current code though).