thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Paulo Neves (Jira)" <j...@apache.org>
Subject [jira] [Comment Edited] (THRIFT-5030) Add possibility for TMessage seqid verification in the processor function
Date Wed, 04 Dec 2019 10:18:00 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-5030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16987711#comment-16987711
] 

Paulo Neves edited comment on THRIFT-5030 at 12/4/19 10:17 AM:
---------------------------------------------------------------

Hello Jens.

Sorry for the delay. I actually think that your proposal makes the most sense and will adapt
accordingly. I did not do it because I am using this code in production and want to align
and verify it for the Thrift project and internally. Also I was preparing the ReverseTunnelServer
submission.
{quote}BTW, I'm still curious when this below could occur. Do you have an example at hand?
{quote}
I do and it is not session management, just demuxing. Imagine you have a single persistent
transport connection to the server that is re-used by multiple RPC clients concurrently. This
is perfectly legitimate with the transport and RPC layer separation. Then imagine that this
single transport connection is fanned out to the multiple active clients. The multiple active
RPC clients need a way to discover if the returned message received is for them. To do so
the message needs to have some kind of identifier.

We already have a seqid so I tried to use the seqid. The problem is when each RPC client starts
more or less at the same time and have the same seqid starting at 0. If the responses of the
RPC server are close to each other it may happen that for example multiple return messages
arrive with the same seqid. If this happens most protocols will fail. On the other hand if
you have a seqid which has a lower probability of collision then we are more likely to succeed
in our call.

You correctly pointed out that a random seqid can only mitigate the collision problem but
not completely eliminate it. For my personal case, my system would just retry if a a protocol
parsing would fail due to an inappropriate message being allowed to be parsed. I see this
not being ideal for the community though, as it has builtin assumptions.

Which leads to me to your proposal being the correct way forward for my case and for the general
use case:
 * It lets the default validator use the sequential seqid starting at 0 as it is the case
now.
 * It also allows the user to set a seqid value of their choosing.

 * 
 ** The value of their choosing can be a random integer like I have in my proposal.
 ** The value can be an integer from a pool of 32 bit numbers managed by the user, ensuring
collision free scope. This pool only needs to be scoped at the transport level.

I will try to provide the testcases alongside the review.


was (Author: ptsneves):
Hello Jens.

Sorry for the delay. I actually think that your proposal makes the most sense and will adapt
accordingly. I did not do it because I am using this code in production and want to align
and verify it for the Thrift project and internally. Also I was preparing the ReverseTunnelServer
submission.
{quote}BTW, I'm still curious when this below could occur. Do you have an example at hand?
{quote}
I do. Imagine you have a single persistent transport connection to the server that is re-used
by multiple RPC clients concurrently. This is perfectly legitimate with the transport and
RPC layer separation. Then imagine that this single transport connection is fanned out to
the multiple active clients. The multiple active RPC clients need a way to discover if the
returned message received is for them. To do so the message needs to have some kind of identifier.

We already have a seqid so I tried to use the seqid. The problem is when each RPC client starts
more or less at the same time and have the same seqid starting at 0. If the responses of the
RPC server are close to each other it may happen that for example multiple return messages
arrive with the same seqid. If this happens most protocols will fail. On the other hand if
you have a seqid which has a lower probability of collision then we are more likely to succeed
in our call.

You correctly pointed out that a random seqid can only mitigate the collision problem but
not completely eliminate it. For my personal case, my system would just retry if a a protocol
parsing would fail due to an inappropriate message being allowed to be parsed. I see this
not being ideal for the community though, as it has builtin assumptions.

Which leads to me to your proposal being the correct way forward for my case and for the general
use case:
 * It lets the default validator use the sequential seqid starting at 0 as it is the case
now.
 * It also allows the user to set a seqid value of their choosing.

 ** The value of their choosing can be a random integer like I have in my proposal.
 ** The value can be an integer from a pool of 32 bit numbers managed by the user, ensuring
collision free scope. This pool only needs to be scoped at the transport level.

I will try to provide the testcases alongside the review.

> Add possibility for TMessage seqid verification in the processor function
> -------------------------------------------------------------------------
>
>                 Key: THRIFT-5030
>                 URL: https://issues.apache.org/jira/browse/THRIFT-5030
>             Project: Thrift
>          Issue Type: Improvement
>          Components: netstd - Library
>            Reporter: Paulo Neves
>            Assignee: Paulo Neves
>            Priority: Major
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> Currently we have a seqid system that is sent from the client to the server, and retrieved
back. The specification says that the seqid returned by the server should be the same sent
by the client. Currently this seems to be the case on the server side, but the client side
never verifies this to be true.
> I have a pull request that changes that situation for netstd. The client side verification
is useful for when a common transport is being used for multiple client calls. This should
be legal as the processor and transport are separate architectural units. If another client
makes an RPC then we may get messages which are not addressed to us. We should have a way
to let the client caller know that such event happened. 
> Another way to do this is to make this verification in a protocol decorator, that completely
manages the seqid by itself. I also have an implementation for this case, but i have not prepared
the pull request yet. Please let me know which approach do you prefer.
> Personally I have gone the way of the protocol decorator as it solves other issues like
seqid collision due to all the TBaseClient initialization starting with seqid == 1. With the
protocol decorator I was then able to fast skip the message which was not replied with the
expected seqid.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Mime
View raw message