Patch for imap.rb -- error handling when waiting for continuation request response

Hallo,

we are using heavily Ruby IMAP library for communication with cyrus imap
server.

And we have discovered then when [ruby] client waits for continuation
request response,
it only accepts request for more data and it hangs [infinite wait] when
it gets error code.

This happends for example if you are trying to store a message in a
folder where you don't
have sufficient access rights.

You send command wint continuation request do the server, tell it which
folder you want do
write to and wait for server to ask you for data. Cyrus will give you
immediately error message.
Maybe the server that the library was tested with would accept whole
message and then give error
so that the library would seem to work right. But giving error code as
soon as possible is perfectly
valid and is also OK according to the RFC.

So following patch agains imap.rb from latest ruby 1.8.3 fixes this
behaviour.

Please tell me, what you thing.

Regards,

--- /tmp/imap.rb Wed Sep 21 18:05:28 2005
+++ /tmp/imap_fixed.rb Wed Sep 21 18:05:18 2005
@@ -902,8 +902,8 @@
        @responses = Hash.new([].freeze)
        @tagged_responses = {}
        @response_handlers = []
- @tagged_response_arrival = new_cond
- @continuation_request_arrival = new_cond
+ @response_arrival = new_cond
+ @continuation_request_arrival = false
        @logout_command_tag = nil
        @debug_output_bol = true

@@ -934,7 +934,7 @@
              case resp
              when TaggedResponse
                @tagged_responses[resp.tag] = resp
- @tagged_response_arrival.broadcast
+ @response_arrival.broadcast
                if resp.tag == @logout_command_tag
                  return
                end
@@ -949,7 +949,8 @@
                  raise ByeResponseError, resp.raw_data
                end
              when ContinuationRequest
- @continuation_request_arrival.signal
+ @continuation_request_arrival = true
+ @response_arrival.broadcast
              end
              @response_handlers.each do |handler|
                handler.call(resp)
@@ -961,10 +962,7 @@
        end
      end

- def get_tagged_response(tag, cmd)
- until @tagged_responses.key?(tag)
- @tagged_response_arrival.wait
- end
+ def pick_up_tagged_response(tag)
        resp = @tagged_responses.delete(tag)
        case resp.name
        when /\A(?:NO)\z/ni
@@ -976,6 +974,13 @@
        end
      end

+ def get_tagged_response(tag, cmd)
+ until @tagged_responses.key?(tag)
+ @response_arrival.wait
+ end
+ pick_up_tagged_response(tag)
+ end

···

+
      def get_response
        buff = ""
        while true
@@ -1009,7 +1014,7 @@
          put_string(tag + " " + cmd)
          args.each do |i|
            put_string(" ")
- send_data(i)
+ send_data(i, tag)
          end
          put_string(CRLF)
          if cmd == "LOGOUT"
@@ -1048,32 +1053,51 @@
        end
      end

- def send_data(data)
+ def put_string_with_continuation(str, tag = nil)
+ # TODO - only one request is allowed to wait for ContinuationRequest at a time.
+
+ @continuation_request_arrival = false
+ put_string("{" + str.length.to_s + "}" + CRLF)
+
+ until (tag and @tagged_responses.key?(tag)) or @continuation_request_arrival
+ @response_arrival.wait
+ end
+
+ unless @continuation_request_arrival
+ # processing Errors
+ pick_up_tagged_response(tag)
+ parse_error('expected continuation request response')
+ end
+
+ put_string(str)
+ end
+
+ def send_data(data, tag = nil)
        case data
        when nil
          put_string("NIL")
        when String
- send_string_data(data)
+ send_string_data(data, tag)
        when Integer
          send_number_data(data)
        when Array
- send_list_data(data)
+ send_list_data(data, tag)
        when Time
          send_time_data(data)
        when Symbol
          send_symbol_data(data)
        else
- data.send_data(self)
+ data.send_data(self, tag)
        end
      end

- def send_string_data(str)
+ def send_string_data(str, tag = nil)
        case str
        when ""
          put_string('""')
        when /[\x80-\xff\r\n]/n
          # literal
- send_literal(str)
+ send_literal(str, tag)
        when /[(){ \x00-\x1f\x7f%*"\\]/n
          # quoted string
          send_quoted_string(str)
@@ -1086,10 +1110,8 @@
        put_string('"' + str.gsub(/["\\]/n, "\\\\\\&") + '"')
      end

- def send_literal(str)
- put_string("{" + str.length.to_s + "}" + CRLF)
- @continuation_request_arrival.wait
- put_string(str)
+ def send_literal(str, tag = nil)
+ put_string_with_continuation(str, tag)
      end

      def send_number_data(num)
@@ -1099,7 +1121,7 @@
        put_string(num.to_s)
      end

- def send_list_data(list)
+ def send_list_data(list, tag = nil)
        put_string("(")
        first = true
        list.each do |i|
@@ -1108,7 +1130,7 @@
          else
            put_string(" ")
          end
- send_data(i)
+ send_data(i, tag)
        end
        put_string(")")
      end
@@ -1324,7 +1346,7 @@
      private_class_method :u8tou16

      class RawData # :nodoc:
- def send_data(imap)
+ def send_data(imap, tag = nil)
          imap.send(:put_string, @data)
        end

@@ -1336,7 +1358,7 @@
      end

      class Atom # :nodoc:
- def send_data(imap)
+ def send_data(imap, tag = nil)
          imap.send(:put_string, @data)
        end

@@ -1348,7 +1370,7 @@
      end

      class QuotedString # :nodoc:
- def send_data(imap)
+ def send_data(imap, tag = nil)
          imap.send(:send_quoted_string, @data)
        end

@@ -1360,8 +1382,8 @@
      end

      class Literal # :nodoc:
- def send_data(imap)
- imap.send(:send_literal, @data)
+ def send_data(imap, tag = nil)
+ imap.send(:send_literal, @data, tag)
        end

        private
@@ -1372,7 +1394,7 @@
      end

      class MessageSet # :nodoc:
- def send_data(imap)
+ def send_data(imap, tag = nil)
          imap.send(:put_string, format_internal(@data))
        end

--
Mgr. Martin Povolný, soLNet, s.r.o.
Technická podpora <hotline@solnet.cz>
telefon: +420/549131233, +420/737743587

ruby-core@ruby-lang.org and http://rubyforge.org/projects/ruby are the best two places to submit patches and bug reports.

···

On 22 Sep 2005, at 22:56, Martin Povolný wrote:

Hallo,

we are using heavily Ruby IMAP library for communication with cyrus imap
server.

And we have discovered then when [ruby] client waits for continuation
request response,
it only accepts request for more data and it hangs [infinite wait] when
it gets error code.

So following patch agains imap.rb from latest ruby 1.8.3 fixes this
behaviour.

--
Eric Hodel - drbrain@segment7.net - http://segment7.net
FEC2 57F1 D465 EB15 5D6E 7C11 332A 551C 796C 9F04

Hi,

Martin Povolný wrote:

And we have discovered then when [ruby] client waits for continuation
request response,
it only accepts request for more data and it hangs [infinite wait] when
it gets error code.

(snip)

So following patch agains imap.rb from latest ruby 1.8.3 fixes this
behaviour.

Your patch works fine, but I don't want to add the `tag' argument
to send_*.
How about this patch?

Index: lib/net/imap.rb

···

===================================================================
RCS file: /var/cvs/src/ruby/lib/net/imap.rb,v
retrieving revision 1.39.2.11
diff -u -r1.39.2.11 imap.rb
--- lib/net/imap.rb 22 Feb 2005 16:58:33 -0000 1.39.2.11
+++ lib/net/imap.rb 25 Sep 2005 01:27:45 -0000
@@ -902,8 +902,8 @@
       @responses = Hash.new(.freeze)
       @tagged_responses = {}
       @response_handlers =
- @tagged_response_arrival = new_cond
- @continuation_request_arrival = new_cond
+ @response_arrival = new_cond
+ @continuation_request = nil
       @logout_command_tag = nil
       @debug_output_bol = true

@@ -934,7 +934,7 @@
             case resp
             when TaggedResponse
               @tagged_responses[resp.tag] = resp
- @tagged_response_arrival.broadcast
+ @response_arrival.broadcast
               if resp.tag == @logout_command_tag
                 return
               end
@@ -949,7 +949,8 @@
                 raise ByeResponseError, resp.raw_data
               end
             when ContinuationRequest
- @continuation_request_arrival.signal
+ @continuation_request = resp
+ @response_arrival.broadcast
             end
             @response_handlers.each do |handler|
               handler.call(resp)
@@ -961,10 +962,14 @@
       end
     end

- def get_tagged_response(tag, cmd)
+ def get_tagged_response(tag)
       until @tagged_responses.key?(tag)
- @tagged_response_arrival.wait
+ @response_arrival.wait
       end
+ return pick_up_tagged_response(tag)
+ end
+
+ def pick_up_tagged_response(tag)
       resp = @tagged_responses.delete(tag)
       case resp.name
       when /\A(?:NO)\z/ni
@@ -1005,7 +1010,7 @@

     def send_command(cmd, *args, &block)
       synchronize do
- tag = generate_tag
+ tag = Thread.current[:net_imap_tag] = generate_tag
         put_string(tag + " " + cmd)
         args.each do |i|
           put_string(" ")
@@ -1019,7 +1024,7 @@
           add_response_handler(block)
         end
         begin
- return get_tagged_response(tag, cmd)
+ return get_tagged_response(tag)
         ensure
           if block
             remove_response_handler(block)
@@ -1088,7 +1093,15 @@

     def send_literal(str)
       put_string("{" + str.length.to_s + "}" + CRLF)
- @continuation_request_arrival.wait
+ while @continuation_request.nil? &&
+ !@tagged_responses.key?(Thread.current[:net_imap_tag])
+ @response_arrival.wait
+ end
+ if @continuation_request.nil?
+ pick_up_tagged_response(Thread.current[:net_imap_tag])
+ raise ResponseError.new("expected continuation request")
+ end
+ @continuation_request = nil
       put_string(str)
     end

Shugo