Aquarium is not thread-safe #39

Closed
paneq opened this Issue Apr 19, 2013 · 6 comments

Comments

Projects
None yet
4 participants
@paneq

paneq commented Apr 19, 2013

require 'aquarium'

class LoggingAspect
  include Aquarium::Aspects

  def initialize(logger)
    @logger = logger
  end

  def apply(auth_usecase)
    Aspect.new :after, methods: [:run], objects: [auth_usecase] do |jp|
      if !jp.context.raised_exception
        project = jp.context.returned_value
        @logger.info("Project authenticated: #{project}")
      else
        auth_key = jp.context.parameters[0]
        @logger.warn("Project failed to authenticate: #{auth_key}")
        @logger.warn("Error: #{jp.context.raised_exception}")
      end
    end
  end
end

class AuthUsecase
  def run(auth_key)
    auth_key
  end
end

a = AuthUsecase.new
l = Logger.new("output.log")

LoggingAspect.new(l).apply(a)

Thread.new do
  loop do
    result = a.run("123")
    msg = result == "123" ? "OK" : "WTF"
    l.info msg
  end
end

Thread.new do
  loop do
    result = a.run("ABC")
    msg = result == "ABC" ? "OK" : "WTF"
    l.info msg
  end
end
> grep "WTF" output.log | wc -l
20118
> grep "OK" output.log | wc -l
29891

Compare it to unaspected version:

class AuthUsecase
  def run(auth_key)
    auth_key
  end
end

a = AuthUsecase.new
l = Logger.new("output.log")

Thread.new do
  loop do
    result = a.run("123")
    msg = result == "123" ? "OK" : "WTF"
    l.info msg
  end
end

Thread.new do
  loop do
    result = a.run("ABC")
    msg = result == "ABC" ? "OK" : "WTF"
    l.info msg
  end
end
> grep "OK" output2.log | wc -l
113326
> grep "WTF" output2.log | wc -l
0

@ghost ghost assigned deanwampler Apr 19, 2013

@andrzejkrzywda

This comment has been minimized.

Show comment
Hide comment
@andrzejkrzywda

andrzejkrzywda Jul 19, 2013

It can be related to this part of code:

https://github.com/deanwampler/Aquarium/blob/master/aquarium/lib/aquarium/aspects/aspect.rb#L435-L457

  #--
  # By NOT dup'ing the join_point, we save about 25% on the overhead! However, we
  # compromise thread safety, primarily because the join_point's context object will be changed.
  # TODO Refactor context out of static join point part.
  # Note that we have to assign the parameters and block to the context object in case
  # the advice calls "proceed" or "invoke_original_join_point" without arguments.
  #++

It seems to be an optimization, that may be less important in places where we care about Thread safety.

It can be related to this part of code:

https://github.com/deanwampler/Aquarium/blob/master/aquarium/lib/aquarium/aspects/aspect.rb#L435-L457

  #--
  # By NOT dup'ing the join_point, we save about 25% on the overhead! However, we
  # compromise thread safety, primarily because the join_point's context object will be changed.
  # TODO Refactor context out of static join point part.
  # Note that we have to assign the parameters and block to the context object in case
  # the advice calls "proceed" or "invoke_original_join_point" without arguments.
  #++

It seems to be an optimization, that may be less important in places where we care about Thread safety.

@deanwampler

This comment has been minimized.

Show comment
Hide comment
@deanwampler

deanwampler Jul 19, 2013

Owner

Thanks for pointing that out. I'll take a look this weekend.
dean

On Fri, Jul 19, 2013 at 6:40 AM, Andrzej Krzywda
notifications@github.comwrote:

It can be related to this part of code:

https://github.com/deanwampler/Aquarium/blob/master/aquarium/lib/aquarium/aspects/aspect.rb#L435-L457

#--

By NOT dup'ing the join_point, we save about 25% on the overhead! However, we

compromise thread safety, primarily because the join_point's context object will be changed.

TODO Refactor context out of static join point part.

Note that we have to assign the parameters and block to the context object in case

the advice calls "proceed" or "invoke_original_join_point" without arguments.

#++

It seems to be an optimization, that may be less important in places where
we care about Thread safety.


Reply to this email directly or view it on GitHubhttps://github.com/deanwampler/Aquarium/issues/39#issuecomment-21243017
.

Dean Wampler, Ph.D.
"Functional Programming for Java Developers",
"Programming Scala", and
"Programming Hive" - all from O'Reilly
twitter: @deanwampler, @chicagoscala
http://polyglotprogramming.com

Owner

deanwampler commented Jul 19, 2013

Thanks for pointing that out. I'll take a look this weekend.
dean

On Fri, Jul 19, 2013 at 6:40 AM, Andrzej Krzywda
notifications@github.comwrote:

It can be related to this part of code:

https://github.com/deanwampler/Aquarium/blob/master/aquarium/lib/aquarium/aspects/aspect.rb#L435-L457

#--

By NOT dup'ing the join_point, we save about 25% on the overhead! However, we

compromise thread safety, primarily because the join_point's context object will be changed.

TODO Refactor context out of static join point part.

Note that we have to assign the parameters and block to the context object in case

the advice calls "proceed" or "invoke_original_join_point" without arguments.

#++

It seems to be an optimization, that may be less important in places where
we care about Thread safety.


Reply to this email directly or view it on GitHubhttps://github.com/deanwampler/Aquarium/issues/39#issuecomment-21243017
.

Dean Wampler, Ph.D.
"Functional Programming for Java Developers",
"Programming Scala", and
"Programming Hive" - all from O'Reilly
twitter: @deanwampler, @chicagoscala
http://polyglotprogramming.com

@andrzejkrzywda

This comment has been minimized.

Show comment
Hide comment
@andrzejkrzywda

andrzejkrzywda Sep 27, 2013

@deanwampler Any news here? We already use aquarium in places where thread-safety doesn't matter, but it would be great to use it also in thread-sensitive environments.

Is there any way we can help you here?

@deanwampler Any news here? We already use aquarium in places where thread-safety doesn't matter, but it would be great to use it also in thread-sensitive environments.

Is there any way we can help you here?

@chicagoscala

This comment has been minimized.

Show comment
Hide comment
@chicagoscala

chicagoscala Sep 29, 2013

Contributor

Hi, Andrzej,

Thanks for bugging me about this. I haven't had any time recently to work
on this, between client commitments and too many conferences recently.
However, I took the time today and have it working.

It was actually a simple one-line change to
Aspect::alias_original_method_text. It now makes a copy of the joinpoint
rather than sharing a mutable one. I needed time to test this and verify
that the performance was acceptable. When I wrote Aquarium circa 2006, the
overhead for copies was 30%! It's now considerably smaller, no doubt due to
improvements in both cruby and hardware.

I have pushed these changes to GitHub. It will be version 0.6.0, which I'll
release as soon as I can. I need to get the jruby tests passing first and
also update the packaging and publishing rake tasks (Unfortunately, since I
don't work very often with Ruby these days, when I need to revisit
Aquarium, I often need to update rake and other stuff, including the
packaging and publishing tools, etc.)

So, I may not have time tomorrow or the next several days (travel), but
worst case, I'll get that done by next weekend.

Yours,
Dean

On Fri, Sep 27, 2013 at 8:22 AM, Andrzej Krzywda
notifications@github.comwrote:

@deanwampler https://github.com/deanwampler Any news here? We already
use aquarium in places where thread-safety doesn't matter, but it would be
great to use it also in thread-sensitive environments.

Is there any way we can help you here?

Reply to this email directly or view it on GitHubhttps://github.com/deanwampler/Aquarium/issues/39#issuecomment-25244458
.

Dean Wampler, Ph.D.
"Functional Programming for Java Developers",
"Programming Scala", and
"Programming Hive" - all from O'Reilly
twitter: @deanwampler, @chicagoscala
http://polyglotprogramming.com

Contributor

chicagoscala commented Sep 29, 2013

Hi, Andrzej,

Thanks for bugging me about this. I haven't had any time recently to work
on this, between client commitments and too many conferences recently.
However, I took the time today and have it working.

It was actually a simple one-line change to
Aspect::alias_original_method_text. It now makes a copy of the joinpoint
rather than sharing a mutable one. I needed time to test this and verify
that the performance was acceptable. When I wrote Aquarium circa 2006, the
overhead for copies was 30%! It's now considerably smaller, no doubt due to
improvements in both cruby and hardware.

I have pushed these changes to GitHub. It will be version 0.6.0, which I'll
release as soon as I can. I need to get the jruby tests passing first and
also update the packaging and publishing rake tasks (Unfortunately, since I
don't work very often with Ruby these days, when I need to revisit
Aquarium, I often need to update rake and other stuff, including the
packaging and publishing tools, etc.)

So, I may not have time tomorrow or the next several days (travel), but
worst case, I'll get that done by next weekend.

Yours,
Dean

On Fri, Sep 27, 2013 at 8:22 AM, Andrzej Krzywda
notifications@github.comwrote:

@deanwampler https://github.com/deanwampler Any news here? We already
use aquarium in places where thread-safety doesn't matter, but it would be
great to use it also in thread-sensitive environments.

Is there any way we can help you here?

Reply to this email directly or view it on GitHubhttps://github.com/deanwampler/Aquarium/issues/39#issuecomment-25244458
.

Dean Wampler, Ph.D.
"Functional Programming for Java Developers",
"Programming Scala", and
"Programming Hive" - all from O'Reilly
twitter: @deanwampler, @chicagoscala
http://polyglotprogramming.com

@deanwampler

This comment has been minimized.

Show comment
Hide comment
Owner

deanwampler commented Sep 29, 2013

Ij

@deanwampler deanwampler reopened this Sep 29, 2013

@deanwampler

This comment has been minimized.

Show comment
Hide comment
@deanwampler

deanwampler Nov 2, 2013

Owner

I released v0.6.0 with this fix.

Owner

deanwampler commented Nov 2, 2013

I released v0.6.0 with this fix.

@deanwampler deanwampler closed this Nov 2, 2013

gordonc added a commit to gordonc/tripgraph that referenced this issue Feb 19, 2014

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