Render with custom import hangs

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 node-sass@3.0.0-beta.5

@saper

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

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

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

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

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

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

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.

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)

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

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

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.

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

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?

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:

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.
A custom importer gets called by the function provided via sass_make_importer()
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.
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.
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).
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.

I put this in my webpack config (CoffeeScript):

# workaround for https://github.com/sass/node-sass/issues/857
# if webpack hangs, increase this number
process.env.UV_THREADPOOL_SIZE=100

That works fine, no need to change the calling scripts.

It works only when another node engine is invoked (I guess that's what webpack is doing). Changing UV_THREADPOOL_SIZE in the already running process has no effect.

@saper

@saper I was wondering about that :-)

Note to self:

backtrace of the hanged thread:

#0  _umtx_op_err ()
    at /usr/src/lib/libthr/arch/amd64/amd64/_umtx_op_err.S:37
#1  0x00000008023a818e in _thr_umtx_timedwait_uint (
    mtx=0x8013190b0, id=<value optimized out>, 
    clockid=<value optimized out>, 
    abstime=<value optimized out>, shared=0)
    at /usr/src/lib/libthr/thread/thr_umtx.c:212
#2  0x00000008023b02de in cond_wait_common (
    cond=<value optimized out>, 
    mutex=<value optimized out>, abstime=0x0, 
    cancel=1)
    at /usr/src/lib/libthr/thread/thr_cond.c:255
#3  0x0000000000cd38e9 in uv_cond_wait ()
#4  0x0000000802dac780 in CallbackBridge<Sass_Import**, void*>::operator() (this=0x803467200, 
    argv=<value optimized out>)
    at callback_bridge.h:133
#5  0x0000000802da89e3 in sass_importer (
    cur_path=0x7fffdf1f59a9 "_partial", 
    cb=0x8034199c0, comp=0x803c1c0c0)
    at ../src/binding.cpp:19
#6  0x00000008039256e6 in Sass::Parser::do_import ()
   from /usr/local/lib/libsass.so.0
#7  0x000000080392bd8e in Sass::Parser::parse_import
    () from /usr/local/lib/libsass.so.0
#8  0x0000000803927914 in Sass::Parser::parse_block_node () from /usr/local/lib/libsass.so.0
#9  0x0000000803925f74 in Sass::Parser::parse_block_nodes () from /usr/local/lib/libsass.so.0
#10 0x0000000803924e31 in Sass::Parser::parse ()
   from /usr/local/lib/libsass.so.0
#11 0x00000008038b1cce in Sass::Context::process_queue_entry () from /usr/local/lib/libsass.so.0
#12 0x00000008038b30fb in Sass::Context::parse_file
    () from /usr/local/lib/libsass.so.0
#13 0x000000080395c759 in sass_compiler_parse ()
   from /usr/local/lib/libsass.so.0
#14 0x000000080395c420 in sass_compile_data_context
    () from /usr/local/lib/libsass.so.0
#15 0x000000080395c550 in sass_compile_file_context
    () from /usr/local/lib/libsass.so.0
#16 0x0000000802dbcdc2 in compile_file (
    fctx=0x8034631e0)
    at ../src/sass_context_wrapper.cpp:22
#17 0x0000000802dbcd57 in compile_it (
    req=0x8034fe7d8)
    at ../src/sass_context_wrapper.cpp:13

Had the same problem, to solve it I've updated my Makefile to have:

serve:
    UV_THREADPOOL_SIZE=100 npm start

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

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

Unfortunately a dead lock, prevents us doing anything.

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 notifications@github.com
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)

PRs welcome
On Jan 21, 2016 12:05 AM, "Wout Mertens" notifications@github.com 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 notifications@github.com
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).

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

Implement queue for node-sass render

add node version to package.json

Navigation Menu

Repository navigation

Render with custom import hangs #857

19 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Render with custom import hangs #857

Description

Activity

xzyfer commented on Apr 13, 2015

saper commented on Apr 13, 2015

xzyfer commented on Apr 13, 2015

xzyfer commented on Apr 13, 2015

saper commented on Apr 13, 2015

xzyfer commented on Apr 13, 2015

xzyfer commented on Apr 13, 2015

saper commented on Apr 13, 2015

xzyfer commented on Apr 13, 2015

saper commented on Apr 14, 2015

saper commented on Apr 15, 2015

mgreter commented on Apr 15, 2015

saper commented on Apr 15, 2015

19 remaining items

wmertens commented on Aug 27, 2015

saper commented on Aug 27, 2015

wmertens commented on Aug 27, 2015

saper commented on Sep 29, 2015

Sapphire64 commented on Oct 2, 2015

wmertens commented on Jan 13, 2016

xzyfer commented on Jan 20, 2016

wmertens commented on Jan 20, 2016

xzyfer commented on Jan 20, 2016

saper commented on Feb 14, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions