threaded_map prevents to catch errors properly #320

Merged
merged 2 commits into from Oct 20, 2012

Conversation

Projects
None yet
2 participants
@heaven
Contributor

heaven commented Oct 19, 2012

Hi, suffering from this issue since update to 4.* version. In my code I have somethinglike this:

users = twitter_request do
  client.users(sites.map(&:username))
end

and

def twitter_request(attempt = 1, &block)
  yield
rescue ::Twitter::Error::TooManyRequests => e
 ...
end

Before it worked just fine, but now, if any error appear in client.users(sites.map(&:username)) it is being handled by the twitter_request method but also raise up and cause the whole process to die.

Writing something like this each time I need to call the API:

users = begin
    client.users(sites.map(&:username))
  rescue ...
  rescue ....
  rescue ...
end

is not too good.

This problem could be solved by adding Thread.abort_on_exception = true to the threaded_map (and I think that would be correct to stop other threads if one fails)

Thank you,
Alex

@sferik

This comment has been minimized.

Show comment Hide comment
@sferik

sferik Oct 19, 2012

Owner

This looks good, except, instead of setting Thread.abort_on_exception = false at the end, it should be reset to whatever it was before the method was called. Would you mind updating the method to look more like this:

def threaded_map
  initial_abort_on_exception = Thread.abort_on_exception
  Thread.abort_on_exception ||= true
  threads = []
  each do |object|
    threads << Thread.new{yield object}
  end
  Thread.abort_on_exception = initial_abort_on_exception
  threads.map(&:value)
end
Owner

sferik commented Oct 19, 2012

This looks good, except, instead of setting Thread.abort_on_exception = false at the end, it should be reset to whatever it was before the method was called. Would you mind updating the method to look more like this:

def threaded_map
  initial_abort_on_exception = Thread.abort_on_exception
  Thread.abort_on_exception ||= true
  threads = []
  each do |object|
    threads << Thread.new{yield object}
  end
  Thread.abort_on_exception = initial_abort_on_exception
  threads.map(&:value)
end
@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Oct 20, 2012

Contributor

Yup, done.

Contributor

heaven commented Oct 20, 2012

Yup, done.

@sferik sferik merged this pull request into sferik:master Oct 20, 2012

@sferik

This comment has been minimized.

Show comment Hide comment
@sferik

sferik Oct 20, 2012

Owner

If you tell me your Twitter user name, I will add it to the CHANGELOG and tweet about this change.

Owner

sferik commented Oct 20, 2012

If you tell me your Twitter user name, I will add it to the CHANGELOG and tweet about this change.

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Oct 20, 2012

Contributor

Hi, my twitter name is aheaven87, but it is being used for development only :) Thank you!

Contributor

heaven commented Oct 20, 2012

Hi, my twitter name is aheaven87, but it is being used for development only :) Thank you!

@sferik

This comment has been minimized.

Show comment Hide comment
@sferik

sferik Oct 20, 2012

Owner

Cool. I added your Twitter handle to the CHANGELOG and mentioned you in a Tweet.

I also refactored the code into a method that takes a block in 7d3ba34.

Owner

sferik commented Oct 20, 2012

Cool. I added your Twitter handle to the CHANGELOG and mentioned you in a Tweet.

I also refactored the code into a method that takes a block in 7d3ba34.

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Oct 20, 2012

Contributor

Thx

Contributor

heaven commented Oct 20, 2012

Thx

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Nov 6, 2012

Contributor

I figured the problem out. Solution we used doesn't always work and I also found that Thread.abort_on_exception is set to true by default but sometimes I still can't catch errors properly. My rescue catch them, but since errors are raised in separate threads, my rescue doesn't prevent them to raise and stop the interpreter.

Forcing abort_on_exception to false do the trick, since there is threads.map(&:value), errors raised in threads will reach main Thread when all threads will done (and they'll be raised in main Thread instead, which allow to catch those errors properly in users code). But I am not sure though if this is logically correct, e.g. if I do client.users(long_user_ids_array) and some threads will fail with http or another error, others will continue to work and errors will be raised only when they all finish.

So, I think would be correct to do something like this:

module Enumerable

  def threaded_map
    twitter_abort_on_exception do
      threads = []
      each do |object|
        threads << Thread.new { twitter_handle_exception(threads) { yield object } }
      end
      threads.map(&:value)
    end
  end

  private

  def twitter_abort_on_exception
    initial_abort_on_exception = Thread.abort_on_exception
    Thread.abort_on_exception = false
    value = yield
    Thread.abort_on_exception = initial_abort_on_exception
    value
  end

  def twitter_handle_exception(threads, &block)
    yield
  rescue => e
    threads.find_all { |t| t != Thread.current }.map(&:kill)
    raise e
  end

end

Best,
Alex

Contributor

heaven commented Nov 6, 2012

I figured the problem out. Solution we used doesn't always work and I also found that Thread.abort_on_exception is set to true by default but sometimes I still can't catch errors properly. My rescue catch them, but since errors are raised in separate threads, my rescue doesn't prevent them to raise and stop the interpreter.

Forcing abort_on_exception to false do the trick, since there is threads.map(&:value), errors raised in threads will reach main Thread when all threads will done (and they'll be raised in main Thread instead, which allow to catch those errors properly in users code). But I am not sure though if this is logically correct, e.g. if I do client.users(long_user_ids_array) and some threads will fail with http or another error, others will continue to work and errors will be raised only when they all finish.

So, I think would be correct to do something like this:

module Enumerable

  def threaded_map
    twitter_abort_on_exception do
      threads = []
      each do |object|
        threads << Thread.new { twitter_handle_exception(threads) { yield object } }
      end
      threads.map(&:value)
    end
  end

  private

  def twitter_abort_on_exception
    initial_abort_on_exception = Thread.abort_on_exception
    Thread.abort_on_exception = false
    value = yield
    Thread.abort_on_exception = initial_abort_on_exception
    value
  end

  def twitter_handle_exception(threads, &block)
    yield
  rescue => e
    threads.find_all { |t| t != Thread.current }.map(&:kill)
    raise e
  end

end

Best,
Alex

heaven added a commit to heaven/twitter that referenced this pull request Nov 6, 2012

#320 - Handle threads exceptions and raise them inside the main Thre…
…ad, so they could be handled in users code.
@sferik

This comment has been minimized.

Show comment Hide comment
@sferik

sferik Dec 7, 2012

Owner

I had to revert this patch: 6de998c

It was causing threading issues and was deemed unnecessary.

Owner

sferik commented Dec 7, 2012

I had to revert this patch: 6de998c

It was causing threading issues and was deemed unnecessary.

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