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

Deprecate reporting methods for silencing output as they aren't thread safe #13392

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@tommeier
Contributor

tommeier commented Dec 19, 2013

As per #13139, this is the deprecation warning methods for these thread unsafe methods.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2013

Member

There are some test failures, and a lot of deprecations in our own test suite 😢. If I got it right, we would keep using that in our own tests, but deprecate as public API, right? So we'll have to cleanup the deprecations in our own tests somehow.

@tommeier mind taking a look at the failures? Maybe we would need to copy these to a test helper or something like that to use on our tests, and leave the public ones deprecated.

Member

carlosantoniodasilva commented Dec 19, 2013

There are some test failures, and a lot of deprecations in our own test suite 😢. If I got it right, we would keep using that in our own tests, but deprecate as public API, right? So we'll have to cleanup the deprecations in our own tests somehow.

@tommeier mind taking a look at the failures? Maybe we would need to copy these to a test helper or something like that to use on our tests, and leave the public ones deprecated.

@tommeier

This comment has been minimized.

Show comment
Hide comment
@tommeier

tommeier Dec 20, 2013

Contributor

Happy to do so @carlosantoniodasilva :) All the failures are just the additional output including deprecation warnings. Where would be a good spot for a test_helper? Considering that we'd want to use the methods in railties and active support (for example), is there a good shared spot?

Contributor

tommeier commented Dec 20, 2013

Happy to do so @carlosantoniodasilva :) All the failures are just the additional output including deprecation warnings. Where would be a good spot for a test_helper? Considering that we'd want to use the methods in railties and active support (for example), is there a good shared spot?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented Jul 15, 2014

Merged at 1fbb5c1...3121412

rafaelfranca added a commit that referenced this pull request Jul 15, 2014

@tommeier tommeier deleted the tommeier:add-deprecation-warnings-for-unsafe-reporting-methods branch Jul 15, 2014

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Nov 13, 2014

Contributor

Is there going to be something to replace this? Some of this code is also duplicated elsewhere in the codebase without the deprecation. I'm not sure why e.g. railties/test/isolation_abstract_unit.rb implements it's own capture.

Also, wouldn't the below be thread-safe due to how it uses pipes and threads? (untested assertion). silencing would just be a matter of not returning the captured output.

    # From episode 029 of Ruby Tapas by Avdi
    # https://rubytapas.dpdcart.com/subscriber/post?id=88
    def capture_output(stream=STDOUT, &block)
      old_stdout = stream.clone
      pipe_r, pipe_w = IO.pipe
      pipe_r.sync    = true
      output         = ""
      reader = Thread.new do
        begin
          loop do
            output << pipe_r.readpartial(1024)
          end
        rescue EOFError
        end
      end
      stream.reopen(pipe_w)
      yield
    ensure
      stream.reopen(old_stdout)
      pipe_w.close
      reader.join
      pipe_r.close
      return output
    end

or e.g. https://github.com/rubygems/rubygems/blob/dd7ca9b2c321f7/lib/rubygems/util.rb#L74

  NULL_DEVICE = defined?(IO::NULL) ? IO::NULL : Gem.win_platform? ? 'NUL' : '/dev/null'

  def self.silent_system(null_device=NULL_DEVICE,&block)
    require 'thread'

    @silent_mutex ||= Mutex.new

    @silent_mutex.synchronize do
      begin
        stdout = STDOUT.dup
        stderr = STDERR.dup

        STDOUT.reopen null_device, 'w'
        STDERR.reopen null_device, 'w'

        return yield
      ensure
        STDOUT.reopen stdout
        STDERR.reopen stderr
        stdout.close
        stderr.close
      end
    end
  end
Contributor

bf4 commented Nov 13, 2014

Is there going to be something to replace this? Some of this code is also duplicated elsewhere in the codebase without the deprecation. I'm not sure why e.g. railties/test/isolation_abstract_unit.rb implements it's own capture.

Also, wouldn't the below be thread-safe due to how it uses pipes and threads? (untested assertion). silencing would just be a matter of not returning the captured output.

    # From episode 029 of Ruby Tapas by Avdi
    # https://rubytapas.dpdcart.com/subscriber/post?id=88
    def capture_output(stream=STDOUT, &block)
      old_stdout = stream.clone
      pipe_r, pipe_w = IO.pipe
      pipe_r.sync    = true
      output         = ""
      reader = Thread.new do
        begin
          loop do
            output << pipe_r.readpartial(1024)
          end
        rescue EOFError
        end
      end
      stream.reopen(pipe_w)
      yield
    ensure
      stream.reopen(old_stdout)
      pipe_w.close
      reader.join
      pipe_r.close
      return output
    end

or e.g. https://github.com/rubygems/rubygems/blob/dd7ca9b2c321f7/lib/rubygems/util.rb#L74

  NULL_DEVICE = defined?(IO::NULL) ? IO::NULL : Gem.win_platform? ? 'NUL' : '/dev/null'

  def self.silent_system(null_device=NULL_DEVICE,&block)
    require 'thread'

    @silent_mutex ||= Mutex.new

    @silent_mutex.synchronize do
      begin
        stdout = STDOUT.dup
        stderr = STDERR.dup

        STDOUT.reopen null_device, 'w'
        STDERR.reopen null_device, 'w'

        return yield
      ensure
        STDOUT.reopen stdout
        STDERR.reopen stderr
        stdout.close
        stderr.close
      end
    end
  end
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 13, 2014

Member

Is there going to be something to replace this?

No.

Some of this code is also duplicated elsewhere in the codebase without the deprecation.

Yes, it is duplicated because the framework still need these methods but we don't want to make them available for the users.

Member

rafaelfranca commented Nov 13, 2014

Is there going to be something to replace this?

No.

Some of this code is also duplicated elsewhere in the codebase without the deprecation.

Yes, it is duplicated because the framework still need these methods but we don't want to make them available for the users.

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Nov 13, 2014

Contributor

Thanks :)

Contributor

bf4 commented Nov 13, 2014

Thanks :)

@pboling

This comment has been minimized.

Show comment
Hide comment
@pboling

pboling Sep 23, 2018

Contributor

@bf4 I have extracted the silence stream functionality into a standalone gem called silent_stream. Should ease the migration from Rails <= 4 to Rails >= 5 for some projects. Also can be used outside Rails.

Contributor

pboling commented Sep 23, 2018

@bf4 I have extracted the silence stream functionality into a standalone gem called silent_stream. Should ease the migration from Rails <= 4 to Rails >= 5 for some projects. Also can be used outside Rails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment