From d450e8a394c9661214d57a7558c3c835297cae25 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Thu, 29 Aug 2019 22:57:16 +1000 Subject: [PATCH 1/3] Clean up Imap.LiteralCommand API Expose value buffer as a RO property, remove redundant method calls, fix call sites. --- .../command/imap-authenticate-command.vala | 4 +- src/engine/imap/command/imap-command.vala | 2 +- .../imap/parameter/imap-list-parameter.vala | 6 +-- .../parameter/imap-literal-parameter.vala | 47 ++++++------------- .../response/imap-fetch-data-decoder.vala | 9 ++-- 5 files changed, 26 insertions(+), 42 deletions(-) diff --git a/src/engine/imap/command/imap-authenticate-command.vala b/src/engine/imap/command/imap-authenticate-command.vala index acf49f3d..c3dfa694 100644 --- a/src/engine/imap/command/imap-authenticate-command.vala +++ b/src/engine/imap/command/imap-authenticate-command.vala @@ -64,7 +64,9 @@ public class Geary.Imap.AuthenticateCommand : Command { // Wait to either get a response or a continuation request yield this.error_lock.wait_async(cancellable); if (this.response_literal != null) { - yield this.response_literal.serialize_data(ser, cancellable); + yield ser.push_literal_data( + this.response_literal.value, cancellable + ); ser.push_eol(cancellable); yield ser.flush_stream(cancellable); } diff --git a/src/engine/imap/command/imap-command.vala b/src/engine/imap/command/imap-command.vala index be7764de..324538e1 100644 --- a/src/engine/imap/command/imap-command.vala +++ b/src/engine/imap/command/imap-command.vala @@ -178,7 +178,7 @@ public class Geary.Imap.Command : BaseObject { // Will get notified via continuation_requested // when server indicated the literal can be sent. yield this.literal_spinlock.wait_async(cancellable); - yield literal.serialize_data(ser, cancellable); + yield ser.push_literal_data(literal.value, cancellable); } } } diff --git a/src/engine/imap/parameter/imap-list-parameter.vala b/src/engine/imap/parameter/imap-list-parameter.vala index 4a6b4859..d580830a 100644 --- a/src/engine/imap/parameter/imap-list-parameter.vala +++ b/src/engine/imap/parameter/imap-list-parameter.vala @@ -217,7 +217,7 @@ public class Geary.Imap.ListParameter : Geary.Imap.Parameter { return stringp; LiteralParameter? literalp = param as LiteralParameter; - if (literalp != null && literalp.get_size() <= MAX_STRING_LITERAL_LENGTH) + if (literalp != null && literalp.value.size <= MAX_STRING_LITERAL_LENGTH) return literalp.coerce_to_string_parameter(); throw new ImapError.TYPE_ERROR("Parameter %d not of type string or literal (is %s)", index, @@ -246,7 +246,7 @@ public class Geary.Imap.ListParameter : Geary.Imap.Parameter { return stringp; LiteralParameter? literalp = param as LiteralParameter; - if (literalp != null && literalp.get_size() <= MAX_STRING_LITERAL_LENGTH) + if (literalp != null && literalp.value.size <= MAX_STRING_LITERAL_LENGTH) return literalp.coerce_to_string_parameter(); throw new ImapError.TYPE_ERROR("Parameter %d not of type string or literal (is %s)", index, @@ -390,7 +390,7 @@ public class Geary.Imap.ListParameter : Geary.Imap.Parameter { public Memory.Buffer? get_as_nullable_buffer(int index) throws ImapError { LiteralParameter? literalp = get_if_literal(index); if (literalp != null) - return literalp.get_buffer(); + return literalp.value; StringParameter? stringp = get_if_string(index); if (stringp != null) diff --git a/src/engine/imap/parameter/imap-literal-parameter.vala b/src/engine/imap/parameter/imap-literal-parameter.vala index a3b1055f..379ed7d7 100644 --- a/src/engine/imap/parameter/imap-literal-parameter.vala +++ b/src/engine/imap/parameter/imap-literal-parameter.vala @@ -1,38 +1,30 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ /** * A representation of an IMAP literal parameter. * - * Because a literal parameter can hold 8-bit data, this is not a descendent of - * {@link StringParameter}, although some times literal data is used to store 8-bit text (for - * example, UTF-8). + * Because a literal parameter can hold 8-bit data, this is not a + * descendent of {@link StringParameter}, although some times literal + * data is used to store 8-bit text (for example, UTF-8). * * See [[http://tools.ietf.org/html/rfc3501#section-4.3]] */ public class Geary.Imap.LiteralParameter : Geary.Imap.Parameter { - private Memory.Buffer buffer; - public LiteralParameter(Memory.Buffer buffer) { - this.buffer = buffer; - } - /** - * Returns the number of bytes in the literal parameter's buffer. - */ - public size_t get_size() { - return buffer.size; - } + /** The value of the literal parameter. */ + public Memory.Buffer value { get; private set; } - /** - * Returns the literal paremeter's buffer. - */ - public Memory.Buffer get_buffer() { - return buffer; + + public LiteralParameter(Memory.Buffer value) { + this.value = value; } /** @@ -45,14 +37,14 @@ public class Geary.Imap.LiteralParameter : Geary.Imap.Parameter { * for transmitting on the wire. */ public StringParameter coerce_to_string_parameter() { - return new UnquotedStringParameter(buffer.get_valid_utf8()); + return new UnquotedStringParameter(this.value.get_valid_utf8()); } /** * {@inheritDoc} */ public override string to_string() { - return "{literal/%lub}".printf(get_size()); + return "{literal/%lub}".printf(this.value.size); } /** @@ -60,17 +52,8 @@ public class Geary.Imap.LiteralParameter : Geary.Imap.Parameter { */ public override void serialize(Serializer ser, GLib.Cancellable cancellable) throws GLib.Error { - ser.push_unquoted_string("{%lu}".printf(get_size()), cancellable); + ser.push_unquoted_string("{%lu}".printf(this.value.size), cancellable); ser.push_eol(cancellable); } - /** - * Serialises the literal parameter data. - */ - public async void serialize_data(Serializer ser, - GLib.Cancellable cancellable) - throws GLib.Error { - yield ser.push_literal_data(buffer, cancellable); - } - } diff --git a/src/engine/imap/response/imap-fetch-data-decoder.vala b/src/engine/imap/response/imap-fetch-data-decoder.vala index 429f4979..8f9f42b9 100644 --- a/src/engine/imap/response/imap-fetch-data-decoder.vala +++ b/src/engine/imap/response/imap-fetch-data-decoder.vala @@ -42,7 +42,7 @@ public abstract class Geary.Imap.FetchDataDecoder : BaseObject { // because this method is called without the help of get_as_string() (which converts // reasonably-length literals into StringParameters), do so here manually try { - if (literalp.get_size() <= ListParameter.MAX_STRING_LITERAL_LENGTH) + if (literalp.value.size <= ListParameter.MAX_STRING_LITERAL_LENGTH) return decode_string(literalp.coerce_to_string_parameter()); } catch (ImapError imap_err) { // if decode_string() throws a TYPE_ERROR, retry as a LiteralParameter, otherwise @@ -197,7 +197,7 @@ public class Geary.Imap.RFC822HeaderDecoder : Geary.Imap.FetchDataDecoder { } protected override MessageData decode_literal(LiteralParameter literalp) throws ImapError { - return new Geary.Imap.RFC822Header(literalp.get_buffer()); + return new Geary.Imap.RFC822Header(literalp.value); } } @@ -207,7 +207,7 @@ public class Geary.Imap.RFC822TextDecoder : Geary.Imap.FetchDataDecoder { } protected override MessageData decode_literal(LiteralParameter literalp) throws ImapError { - return new Geary.Imap.RFC822Text(literalp.get_buffer()); + return new Geary.Imap.RFC822Text(literalp.value); } protected override MessageData decode_nil(NilParameter nilp) throws ImapError { @@ -221,7 +221,6 @@ public class Geary.Imap.RFC822FullDecoder : Geary.Imap.FetchDataDecoder { } protected override MessageData decode_literal(LiteralParameter literalp) throws ImapError { - return new Geary.Imap.RFC822Full(literalp.get_buffer()); + return new Geary.Imap.RFC822Full(literalp.value); } } - From 4c56ff34369f756a120631368280a1bbf7b2087e Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Thu, 29 Aug 2019 23:11:32 +1000 Subject: [PATCH 2/3] Convert Imap.Serialiser.push_literal_data to take generic param Rather than taking a Literal argument, take a data array instead. This allows more flexibility in sending literal data to the server. --- src/engine/imap/command/imap-authenticate-command.vala | 2 +- src/engine/imap/command/imap-command.vala | 4 +++- src/engine/imap/transport/imap-serializer.vala | 10 +++++----- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/engine/imap/command/imap-authenticate-command.vala b/src/engine/imap/command/imap-authenticate-command.vala index c3dfa694..041de90e 100644 --- a/src/engine/imap/command/imap-authenticate-command.vala +++ b/src/engine/imap/command/imap-authenticate-command.vala @@ -65,7 +65,7 @@ public class Geary.Imap.AuthenticateCommand : Command { yield this.error_lock.wait_async(cancellable); if (this.response_literal != null) { yield ser.push_literal_data( - this.response_literal.value, cancellable + this.response_literal.value.get_uint8_array(), cancellable ); ser.push_eol(cancellable); yield ser.flush_stream(cancellable); diff --git a/src/engine/imap/command/imap-command.vala b/src/engine/imap/command/imap-command.vala index 324538e1..361bbca1 100644 --- a/src/engine/imap/command/imap-command.vala +++ b/src/engine/imap/command/imap-command.vala @@ -178,7 +178,9 @@ public class Geary.Imap.Command : BaseObject { // Will get notified via continuation_requested // when server indicated the literal can be sent. yield this.literal_spinlock.wait_async(cancellable); - yield ser.push_literal_data(literal.value, cancellable); + yield ser.push_literal_data( + literal.value.get_uint8_array(), cancellable + ); } } } diff --git a/src/engine/imap/transport/imap-serializer.vala b/src/engine/imap/transport/imap-serializer.vala index 2df8b484..36dafca9 100644 --- a/src/engine/imap/transport/imap-serializer.vala +++ b/src/engine/imap/transport/imap-serializer.vala @@ -103,14 +103,14 @@ public class Geary.Imap.Serializer : BaseObject { /** * Writes literal data to the output stream. */ - public async void push_literal_data(Memory.Buffer buffer, + public async void push_literal_data(uint8[] buffer, GLib.Cancellable? cancellable = null) throws GLib.Error { - yield this.output.splice_async( - buffer.get_input_stream(), - OutputStreamSpliceFlags.NONE, + yield this.output.write_all_async( + buffer, Priority.DEFAULT, - cancellable + cancellable, + null ); } From 4b84616d8263857f83cdbe53135a5d24ecc2b9d1 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Thu, 29 Aug 2019 23:13:48 +1000 Subject: [PATCH 3/3] Write literal data in chunks when sending to server By splitting literal writes into apprpriately sized chunks, the command's response timer can be updated so that the command doesn't fail due to a timeout as large values are being sent. Fixes #543 --- src/engine/imap/command/imap-command.vala | 42 +++++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/src/engine/imap/command/imap-command.vala b/src/engine/imap/command/imap-command.vala index 361bbca1..570fe3ba 100644 --- a/src/engine/imap/command/imap-command.vala +++ b/src/engine/imap/command/imap-command.vala @@ -178,9 +178,45 @@ public class Geary.Imap.Command : BaseObject { // Will get notified via continuation_requested // when server indicated the literal can be sent. yield this.literal_spinlock.wait_async(cancellable); - yield ser.push_literal_data( - literal.value.get_uint8_array(), cancellable - ); + + // Buffer size is dependent on timeout, since we + // need to ensure we can send a full buffer before + // the timeout is up. v.92 56k baud modems have + // theoretical max upload of 48kbit/s and GSM 2G + // 40kbit/s, but typical is usually well below + // that, so assume a low end of 1kbyte/s. Hence + // buffer size needs to be less than or equal to + // (response_timeout * 1)k, rounded down to the + // nearest power of two. + uint buf_size = 1; + while (buf_size <= this.response_timeout) { + buf_size <<= 1; + } + buf_size >>= 1; + + uint8[] buf = new uint8[buf_size * 1024]; + GLib.InputStream data = literal.value.get_input_stream(); + try { + while (true) { + size_t read; + yield data.read_all_async( + buf, Priority.DEFAULT, cancellable, out read + ); + if (read <= 0) { + break; + } + + buf.length = (int) read; + yield ser.push_literal_data(buf, cancellable); + this.response_timer.start(); + } + } finally { + try { + yield data.close_async(); + } catch (GLib.Error err) { + // Oh well + } + } } } }