[PATCH 1 of 3 V2] bundle2: add separate handling for error part creation

[Pierre-Yves David]

On 04/07/2017 09:52 PM, Siddharth Agarwal wrote:
> On 4/6/17 05:50, Pierre-Yves David wrote:
>> While I understand the need for unbounded generic error:Abort, I not
>> sure we should extend this to pushraced and unsupported content one.
>> They have defined enough use case where limiting the message and hint
>> to 255 char seems reasonable (in the exception themselves).
>> Limiting the scope of this more complex creation and handling to
>> error:Abort.
>>
>> What do you think ?
>
>
> My view is that if we're doing this for abort anyway, we might as well
> do this for the other part types.
>
> * unsupportedcontent is arbitrary so it can easily go beyond 255 when
> you concatenate the params together. A misbehaving client can easily
> crash the server that way.

We could decide to truncate the parameters we keep it under 255. That 
value is mostly informative.

> * pushraced is less likely but there's still a non-zero chance that a
> confluence of events can cause this.

I'm not sure how. They are a single raise of PushRaced and it is fixed size

> I think the incremental amount of work required to support it is minimal, so we might as well do it in my
> opinion.

My main reservation here is that it increase the complexity of the 
protocol. So all new implementation (server and client) side of it will 
have to deal with the extra complexity for errors that do not really 
needs it (eg: PushRaced). I usually lead toward keeping things simpler 
as long as we do not need the extra complexity.

On the other hand, I see your point and I understand some of it value, 
but it has not won me over yet, I did not want to delay my reply 
further. (as result you are provided with my raw thinking.)

I wonder what others (especially Greg) thinks.

>> I think we could get away with a simpler way to add the long message
>> to the payload.
>>
>> You current approach implies packing the parameters name and length
>> using our own binary encoding into the payload. This imply tedious
>> binary manipulation on both end.
>> Instead we could leverage more of the bundle2 format. In case of too
>> long message or hint, we add a corresponding 'message_long' or
>> 'hint_long' advisory parameter to the part. This parameter contains a
>> 'start:end' range of bytes in the paylog containing the message. That
>> way most of the binary encoding is handled by the native bundle2
>> machinery and less work is needed on the receiving side.
>>
>> What do you think ?
>
> It'll probably lead to simpler code overall. I like it.
>
> I would want to store the full message in the payload though, as we're
> truncating the message in params for older clients in a friendly-ish way.

If I understand correctly, this match my proposal. Let me clarify to 
make sure we are on the same page.

You get two parameters:
   message="something a bit too lo..."
   message_long=<0><24>
and Payload:
   something a bit too long.

-- 
Pierre-Yves David