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

sass --embedded doesn't reliably exit when closing stdin #2001

Closed
bep opened this issue Jun 8, 2023 · 9 comments · Fixed by #2007
Closed

sass --embedded doesn't reliably exit when closing stdin #2001

bep opened this issue Jun 8, 2023 · 9 comments · Fixed by #2007
Labels
bug embedded Issues about the embedded compiler needs info

Comments

@bep
Copy link

bep commented Jun 8, 2023

I have migrated godartsass to v2 of the protocol, which was mostly smooth sailing.

But I have one test that is now flaky (fails about 50% of the time). I never had this test fail on dart-sass-embedded.

The short story is that I

@bep bep changed the title sass --embedded doesn't reliably exit when closing stdin on Ubuntu sass --embedded doesn't reliably exit when closing stdin Jun 8, 2023
@ntkme
Copy link
Contributor

ntkme commented Jun 8, 2023

I also have issue on Linux on GitHub Actions. In my case it looks like a lockup that randomly happens in the middle of the test. On Windows and Mac CI it never happened.

Somehow I cannot reproduce it on my local Linux box. On GitHub Actions it happens on Ubuntu runner about every 10 tests. I ran the same test in a loop for one hour on my local Linux box and it never happens.

As for the shutdown issue @bep is facing, it never happened for me.

I suspect there might be some Linux specific bugs in Dart VM itself.

@bep
Copy link
Author

bep commented Jun 8, 2023

@ntkme I initially reported this as a "Ubuntu issue", but I have since had this issue on MacOS, too (on GitHub). I have worked around this by sending an interrupt to the process on GOOS != "windows".

@ntkme
Copy link
Contributor

ntkme commented Jun 8, 2023

OK. I finally got to the bottom of the lockup I'm facing, it is a race condition: #2004

@nex3
Copy link
Contributor

nex3 commented Jun 9, 2023

@bep Can you confirm whether this is the same issue as #2004—that is, does it go away if you avoid reusing the same compilation ID?

@nex3 nex3 added bug needs info embedded Issues about the embedded compiler labels Jun 9, 2023
@bep
Copy link
Author

bep commented Jun 9, 2023

@bep I have double checked and

@ntkme
Copy link
Contributor

ntkme commented Jun 9, 2023

If I read your test correctly you are sending lots of requests and shutting down while requests are still inflight?

From what I can tell if the stdin is closed after all requests are completed, the compiler does shutdown correctly.

@ntkme
Copy link
Contributor

ntkme commented Jun 9, 2023

I can confirm the same bug in ruby host as well. Here is a reliable reproduction:

# frozen_string_literal: true

require 'sass-embedded'

embedded = Sass::Embedded.new

threads = []

10.times do
  threads << Thread.new do
    puts embedded.compile_string(
      'a { b: foo() }',
      functions: {
        'foo()' => lambda do |*|
          Sass::Value::Null::NULL
        end
      }
    ).css
  rescue StandardError => e
    puts e
  end
end

puts 'closing'
embedded.close
puts 'closed'

threads.each(&:join)

@ntkme
Copy link
Contributor

ntkme commented Jun 9, 2023

This is a race condition during shutdown, see #2007.

@bep
Copy link
Author

bep commented Jun 9, 2023

If I read your test correctly you are sending lots of requests and shutting down while requests are still inflight?

That's correct, it's a race/robustness test on my side, but I can envision how certain server applications would not care too much about inflight requests when shutting down. Also, this implementation is behaving exactly like the official Go RPC library in this department: On Close, stdin is closed and all local pending requests are cancelled, with an assumption that the other end of the RPC connection is doing the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug embedded Issues about the embedded compiler needs info
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants