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

Fix thread-safety for mixin recursions check #2286

Merged
merged 1 commit into from
Jan 5, 2017

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Jan 5, 2017

Keeping the counter in a static exposed some weird behavior in sass/node-sass#1669 (comment). This PR should fix that particular issue. I believe this is due to thread safety. Not sure how they do it, but the behavior we see is the following:

static unsigned int recursions = 0;
mixin() {
  if (recursions > 500) {
    throw "stack level too deep";
  }
  recursions ++;
  ... do more work
  ... may call mixin() again
  recursions --;
}

At the decrement, recursions at some point contains a 0, therefore the decrement will overflow the unsigned value to 4294967295, which will then trigger the error check. For me this can only mean that the static variable got initialized again on the same thread. Moving the variable to the class will make sure that every context uses their own memory for this counter. Still, LibSass is not guaranteed or considered to be 100% thread safe (mostly due to the lack of anybody really verifying it).

@mgreter
Copy link
Contributor Author

mgreter commented Jan 5, 2017

A simpler reproduction of the error:

var sass = require('node-sass');
function render() {
  sass.render({
    data: [
      "@mixin test2() {",
      "  foo: bar;",
      "}",
      "@mixin test() {",
      "  @for $i from 1 to 100000 {",
      "  }",
      "}",
      "foo {",
      "  @include test2;",
      "  @include test;",
      "  @include test2;",
      "}",
    ].join("\n"),
  }, function(error, result) {
    if (!result) console.log('error ', error);
  });
}

for (var i = 0; i < 100; i ++) {
  setTimeout(render, Math.random() * 1000);
}

FWIW: I always aim to be thread safe in regard of contexts. Multiple contexts/compilers should be safe to use even on the same thread, which is what node-sass obviousely does. Since we don't share anything between them, this should be quite easy to achieve, but nonetheless there is no real guarantee that no side effects hide in the source code! This one was really just badly implemented!

@mgreter mgreter merged commit b667cac into sass:master Jan 5, 2017
@xzyfer xzyfer added this to the 3.4.3 milestone Jan 5, 2017
@xzyfer
Copy link
Contributor

xzyfer commented Jan 7, 2017

Back ported to 3.4 in 06aea01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants