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

[Martin Povolný]

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 <hot...@solnet.cz>
telefon: +420/549131233, +420/737743587

[Eric Hodel]

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.

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

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

[Shugo Maeda]

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