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

2017-02-21 Thread gadLinux 
Github user gadLinux commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1200#discussion_r102329070
  
--- 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 --

Suppose a protocol or transport that add a separator to the packet to, for 
example, add a header of encryption. And the separator is the same. It may be 
confused because found twice, once for the protocol and another for the 
multiplexed. Changing the multiplexed separator will fix the possible issue. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

2017-02-21 Thread gadLinux 
Github user gadLinux commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1200#discussion_r102326459
  
--- Diff: lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c ---
@@ -549,12 +549,27 @@ thrift_protocol_init (ThriftProtocol *protocol)
 }
 
 static void
+thrift_protocol_dispose (GObject *gobject)
+{
+  ThriftProtocol *self = THRIFT_PROTOCOL (gobject);
+
+  g_clear_object(>transport);
+
+  /* Always chain up to the parent class; there is no need to check if
+   * the parent class implements the dispose() virtual function: it is
+   * always guaranteed to do so
+   */
+  G_OBJECT_CLASS (thrift_protocol_parent_class)->dispose(gobject);
--- End diff --

Yes I forgot that! Sorry.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

2017-02-21 Thread gadLinux 
Github user gadLinux commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1200#discussion_r102328202
  
--- Diff: lib/java/test/org/apache/thrift/test/TestServer.java ---
@@ -190,7 +193,9 @@ public static void main(String [] args) {
 if (protocol_type.equals("binary")) {
 } else if (protocol_type.equals("compact")) {
 } else if (protocol_type.equals("json")) {
-} else if (protocol_type.equals("multiplexed")) {
+} else if (protocol_type.equals("multi")) {
--- End diff --

I don't know much about tests. But you can get crazy here if there's a lot 
of procotols. 
I suggest what I did. Adding a prefix and sufix. The prefix can be matched 
against multiplexed (multi) the suffix is the name of the protocol of the 
underlaying implementation. 
This convention doesn't add anything to the code. And makes you be ready 
for the multiplexed whether a new protocol is added with no effort. 
Convention over configuration.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

2017-02-21 Thread gadLinux 
Github user gadLinux commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1200#discussion_r102327046
  
--- Diff: lib/c_glib/src/thrift/c_glib/transport/thrift_socket.h ---
@@ -50,11 +50,8 @@ struct _ThriftSocket
 
   /* private */
   gchar *hostname;
-  gshort port;
+  guint port;
   int sd;
-  guint8 *buf;
--- End diff --

This is the default implementation of the socket. Are you sure about it? If 
this is changed I suppose this should be done in it's own ticket.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

2017-02-21 Thread gadLinux 
Github user gadLinux commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1200#discussion_r102326250
  
--- Diff: lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c ---
@@ -61,15 +61,15 @@ thrift_protocol_set_property (GObject *object, guint 
property_id,
   switch (property_id)
   {
 case PROP_THRIFT_PROTOCOL_TRANSPORT:
-  protocol->transport = g_value_get_object (value);
--- End diff --

Why you duplicate it? The underlaying transport should live as long as the 
multiplexed one. And must be destroyed after protocol is destroyed. Duplicating 
the transport may lead to object references hold and maybe memory freeing 
problems. 
I think this property must hold a reference to it and not a copy. 
The copy can lead to memory freeing problems.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

2017-02-21 Thread gadLinux 
Github user gadLinux commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1200#discussion_r102326623
  
--- Diff: lib/c_glib/src/thrift/c_glib/server/thrift_server.c ---
@@ -76,22 +76,22 @@ thrift_server_set_property (GObject *object, guint 
property_id,
   switch (property_id)
   {
 case PROP_THRIFT_SERVER_PROCESSOR:
-  server->processor = g_value_get_object (value);
--- End diff --

Same here. Are you sure you must duplicate them?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

2017-02-21 Thread gadLinux 
Github user gadLinux commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1200#discussion_r102325143
  
--- 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 --

Why you did remove the ability to select the separator. It seems that Java 
and maybe others support it. Think that some protocols may required to change 
it to something special to make it work. I would leave it.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---