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

Threading Bug in Paginator.rb Helper #164

Closed
johnthethird opened this Issue Sep 16, 2011 · 5 comments

Comments

Projects
None yet
5 participants
@johnthethird
Copy link

johnthethird commented Sep 16, 2011

It seems like https://github.com/amatsuda/kaminari/blob/master/lib/kaminari/helpers/paginator.rb#L58 has some hacky code that does not play well when running on JRuby in threadsafe mode. As a work around, patching it to this in an initializer fixed the problem.

module Kaminari
  module Helpers
    Paginator.class_eval do
      def to_s
        super @window_options.merge(@options).merge :paginator => self
      end
    end
  end
end
@godfat

This comment has been minimized.

Copy link

godfat commented Nov 13, 2012

We're using this patch now. We should never do dirty hack like that.

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented Mar 15, 2013

Couldn't you just put the method in a Thread so that it works across all rubies according to each's understanding of Threads?

def to_s
  Thread.new do
     # stuff
  end
end

bf4 added a commit to bf4/kaminari that referenced this issue Mar 29, 2013

[kaminari#164] Make to_s in paginator threadsafe
Make explicit what the paginator to_s is doing
In a high-load environment, may not
repress logging consistently as desired

bf4 added a commit to bf4/kaminari that referenced this issue Mar 29, 2013

[kaminari#164] Make to_s in paginator threadsafe
Make explicit what the paginator to_s is doing
In a high-load environment, may not
repress logging consistently as desired

amatsuda added a commit that referenced this issue May 2, 2013

Merge pull request #374 from bf4/164-threadsafe-paginator
[#164] Make to_s in paginator threadsafe
@yuki24

This comment has been minimized.

Copy link
Member

yuki24 commented Jun 18, 2013

I believe this issue solved by #374. Thanks!

@yuki24 yuki24 closed this Jun 18, 2013

@san

This comment has been minimized.

Copy link

san commented Aug 26, 2013

It does appear that this issue has been resolved by #374. My question is, do we have a release of the gem after the merge?

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented Aug 27, 2013

See #412

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