[GitHub] thrift pull request #1200: THRIFT-3706: glib client/java server/crosstest mu...

[jeking3 <gi...@git.apache.org>]

Github user jeking3 commented on a diff in the pull request:

    https://github.com/apache/thrift/pull/1200#discussion_r102356256
  
    --- Diff: lib/c_glib/src/thrift/c_glib/protocol/thrift_multiplexed_protocol.c ---
    @@ -42,146 +41,119 @@ static GParamSpec *thrift_multiplexed_protocol_obj_properties[PROP_THRIFT_MULTIP
     
     gint32
     thrift_multiplexed_protocol_write_message_begin (ThriftMultiplexedProtocol *protocol,
    -		const gchar *name, const ThriftMessageType message_type,
    -		const gint32 seqid, GError **error)
    +    const gchar *name, const ThriftMessageType message_type,
    +    const gint32 seqid, GError **error)
     {
    -	gint32 ret;
    -	gchar *service_name = NULL;
    -	g_return_val_if_fail (THRIFT_IS_MULTIPLEXED_PROTOCOL (protocol), -1);
    +  gint32 ret;
    +  gchar *service_name = NULL;
    +  g_return_val_if_fail (THRIFT_IS_MULTIPLEXED_PROTOCOL (protocol), -1);
     
    -	ThriftMultiplexedProtocol *self = THRIFT_MULTIPLEXED_PROTOCOL (protocol);
    -	ThriftMultiplexedProtocolClass *multiplexClass = THRIFT_MULTIPLEXED_PROTOCOL_GET_CLASS(self);
    -	ThriftProtocolClass *cls = THRIFT_PROTOCOL_CLASS (multiplexClass);
    +  ThriftMultiplexedProtocol *self = THRIFT_MULTIPLEXED_PROTOCOL (protocol);
    +  ThriftMultiplexedProtocolClass *multiplexClass = THRIFT_MULTIPLEXED_PROTOCOL_GET_CLASS(self);
    +  ThriftProtocolClass *cls = THRIFT_PROTOCOL_CLASS (multiplexClass);
     
    -	if( (message_type == T_CALL || message_type == T_ONEWAY) && self->service_name != NULL) {
    -		service_name = g_strdup_printf("%s%s%s", self->service_name, self->separator, name);
    +  if( (message_type == T_CALL || message_type == T_ONEWAY) && self->service_name != NULL) {
    +    service_name = g_strdup_printf("%s%s%s", self->service_name, THRIFT_MULTIPLEXED_PROTOCOL_DEFAULT_SEPARATOR, name);
    +  }else{
    +    service_name = g_strdup(name);
    +  }
     
    -	}else{
    -		service_name = g_strdup(name);
    -	}
    +  // relay to the protocol_decorator
    +  ret = thrift_protocol_decorator_write_message_begin(protocol, service_name, message_type, seqid, error);
     
    -	// relay to the protocol_decorator
    -	ret = thrift_protocol_decorator_write_message_begin(protocol, service_name, message_type, seqid, error);
    +  g_free(service_name);
     
    -	g_free(service_name);
    -
    -	return ret;
    +  return ret;
     }
     
     
    -
    -
     static void
     thrift_multiplexed_protocol_set_property (GObject      *object,
    -		guint         property_id,
    -		const GValue *value,
    -		GParamSpec   *pspec)
    +    guint         property_id,
    +    const GValue *value,
    +    GParamSpec   *pspec)
     {
    -	ThriftMultiplexedProtocol *self = THRIFT_MULTIPLEXED_PROTOCOL (object);
    -
    -	switch (property_id)
    -	{
    -	case PROP_THRIFT_MULTIPLEXED_PROTOCOL_SERVICE_NAME:
    -		if(self->service_name!=NULL)
    -			g_free (self->service_name);
    -		self->service_name= g_value_dup_string (value);
    -		break;
    -
    -	case PROP_THRIFT_MULTIPLEXED_PROTOCOL_SEPARATOR:
    --- End diff --
    
    Given it is a Thrift Protocol Decorator and the field in question can only be a string containing the name of the multiplexed service and colon and name of the original call, I don't see how it would be possible for any protocol to be confused by this always being a colon.  Protocols never see it.  In the decoding phase the call name is a string, so we will always read a string length and then a set of characters, and we will always pass this along to the Multiplexed Processor.  What you suggest means that a "string read" method for a protocol would be confused by a colon which shouldn't ever happen.
    
    In theiry it might be possible for a Transport to be confused, however if a Transport is even looking at the content of the payload at all that is a poorly written transport.  So as I see it, using a fixed string across all languages seems quite reasonable.
    
    Now that said, I didn't check the MultiplexedProtocol implementations to make sure they throw an exception if a colon is used in the multiplexed service name.  That should probably be a requirement for all implementations.
    
    In this case, allowing the colon separator to be changed to something else, seems like too much complexity for no gain.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---