fix mruby redis bugs

nalramli · Jun 13, 2017 · c904a00 · c904a00
1 parent 64bc23d
commit c904a00
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 42 deletions.
diff --git a/lib/handler/mruby/redis.c b/lib/handler/mruby/redis.c
@@ -196,10 +196,6 @@ static void on_redis_command(redisReply *_reply, void *_ctx, int err, const char
         reply = mrb_exc_new(mrb, error_klass, errstr, strlen(errstr));
     }
 
-    if (mrb_nil_p(reply)) {
-        return;
-    }
-
     if (mrb_nil_p(ctx->receiver)) {
         mrb_value runner = mrb_funcall(mrb, ctx->refs.command, "_on_reply", 1, reply);
         h2o_mruby_assert(mrb);

diff --git a/misc/mruby_config.rb b/misc/mruby_config.rb
@@ -10,7 +10,7 @@
     toolchain :gcc
   end
 
-  # enable_debug
+  enable_debug
 
   # use mrbgems
   Dir.glob("../mruby-*/mrbgem.rake") do |x|

diff --git a/share/h2o/mruby/redis.rb b/share/h2o/mruby/redis.rb
@@ -18,8 +18,8 @@ def ensure_connected
       yield
     end
 
-    STREAMING_COMMANDS = %w(subscribe psubscribe monitor).map {|c| [c, true] }.to_h
-    NO_REPLY_COMMANDS = %w(unsubscribe).map {|c| [c, true] }.to_h
+    STREAMING_COMMANDS = %w(subscribe psubscribe).map {|c| [c, true] }.to_h
+    NO_REPLY_COMMANDS  = %w(unsubscribe punsubscribe).map {|c| [c, true] }.to_h
     def _command_class(c, block)
       dc = c.to_s.downcase
       if STREAMING_COMMANDS.include?(dc)
@@ -30,14 +30,17 @@ def _command_class(c, block)
         end
       elsif NO_REPLY_COMMANDS.include?(dc)
         Command::NoReply
+      elsif dc.to_sym == :exec
+        Command::OneShot::Exec
       else
         Command::OneShot
       end
     end
 
     def call(*command_args, &block)
+      command_class = _command_class(command_args[0], block)
       ensure_connected do
-        __call(command_args, _command_class(command_args[0], block), &block)
+        __call(command_args, command_class, &block)
       end
     end
 
@@ -64,6 +67,9 @@ def initialize(args, &block)
       end
 
       class OneShot < Command
+        def _check_reply(reply)
+          raise reply if reply.kind_of?(RuntimeError)
+        end
         def _on_reply(reply)
           @reply = reply
           nil
@@ -72,11 +78,22 @@ def join
           if !@reply
             @reply = _h2o__redis_join_reply(self)
           end
-          if @reply.kind_of?(RuntimeError)
-            raise @reply
-          end
+          _check_reply(@reply)
           @reply
         end
+
+        # exec may contain error reply in array reply, or nil reply when watch failed, so have to check it
+        class Exec < OneShot
+          def _check_reply(reply)
+            if reply.nil?
+              raise CommandError.new('transaction was aborted', self)
+            end
+            super(reply)
+            if reply.kind_of?(Array)
+              reply.each {|child| super(child) }
+            end
+          end
+        end
       end
 
       class NoReply < Command
@@ -95,6 +112,7 @@ def initialize(*args, &block)
             super(*args, &block)
             @checker = proc {|reply|
               raise reply if reply.kind_of?(RuntimeError)
+              @@passthru
             }
           end
 
@@ -163,61 +181,58 @@ def _on_reply(reply)
           end
           def join
             reply = _replies.shift || _h2o__redis_join_reply(self)
-            if reply.kind_of?(RuntimeError)
-              raise reply
-            end
+            raise reply if reply.kind_of?(RuntimeError)
             reply
           end
         end
 
       end
     end
 
-    def multi
-      res = call(:MULTI)
-      if ! block_given?
-        return res
-      end
-
+    def _do_block_ensuring(block, &ensuring)
+      success = false
       begin
-        yield self
+        block.call(self)
+        success = true
       rescue ConnectionError => e
-        raise
-      rescue StandardError => e
-        discard
+        # if connection error happens, discard / unwatch are not needed anymore
         raise e
+      ensure
+        # to provide original exception information, pass through using ensure, not re-raise.
+        # are there more smart ways?
+        unless success
+          begin
+            ensuring.call
+          end
+        end
       end
+    end
 
+    def multi(&block)
+      command = call(:MULTI)
+      return command unless block
+      _do_block_ensuring(block) { discard }
       exec
     end
 
-    def watch(*keys)
-      res = call(:WATCH, *keys)
-      if !block_given?
-        return res
-      end
-
-      begin
-        yield self
-      rescue ConnectionError => e
-        raise
-      rescue StandardError => e
-        unwatch
-        raise e
-      end
+    def watch(*keys, &block)
+      command = call(:WATCH, *keys)
+      return command unless block
+      _do_block_ensuring(block) { unwatch }
+      command
     end
 
     def quit
       begin
-        res = call(:QUIT)
+        command = call(:QUIT)
       rescue ConnectionError
-        res = 'OK'
+        command = Command::NoReply.new
       end
 
       if block_given?
-        yield res
+        yield command
       else
-        res
+        command
       end
     end