[GitHub] thrift pull request #1200: THRIFT-3706: glib client/java server/crosstest mu...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1200: THRIFT-3706: glib client/java server/crosstest mu...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1200#discussion_r102356814 --- 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 -- "should" live as long. Why should we impose a requirement that the consuming application, using an object oriented-ish language like glib, be required to ensure the underlying transport outlives the protocol that is consuming it? The copy should not lead to memory issues because the dispose method in this class unreferences and clears the pointer. This means the consuming application does a g_object_new, that's one reference. The protocol consumes it, that's a second reference. If the consuming application were to unref the transport, it would crash. But by using reference counts on objects the way they were intended to be used, a consuming application could quite literally unref everything but the server before serve() and then unref the server when it returns, and there would be no leaks or problems. I don't think it's a good practice to force the consuming application to guarantee the lifetime of pointers that reference each-other when the object library itself provides reference counting and garbage collection. That's why I changed it here and in other classes like ThriftServer. --- 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/crosstest mu...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1200#discussion_r102356989 --- 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 -- "buf" is not used anywhere in thrift_socket.c and it was marked as private. This means it was there for no reason that's useful. --- 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/crosstest mu...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1200#discussion_r102357395 --- 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 did it this way to work within the spec:impl naming convention that currently exists in make cross. See tests.json, specifically "binary:accel" or "compact:accelc". I wanted to follow the pattern that already existed in the test suite so we have a single use pattern, not two. In the end the behavior is mostly the same, except by using "multi:binary" on the java server and "binary:multi" on the c_glib client, we end up testing: c_glib (binary client) => java (multi server) as well as c_glib (multi client wrapping binary) => java (multi server wrapping binary). Using the "multiplexed-binary" naming convention would not have leveraged the existing logic in crosstest/collect.py to make this happen. --- 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/crosstest mu...
Github user gadLinux commented on a diff in the pull request: https://github.com/apache/thrift/pull/1200#discussion_r102411428 --- 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 -- Ok. Then I agree. --- 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/crosstest mu...
Github user gadLinux commented on a diff in the pull request: https://github.com/apache/thrift/pull/1200#discussion_r102412439 --- 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 -- Ok. As long as the reference count is guaranteed to be well managed. And it seems because you added the finalize method it should be okay. But I think this breaks RAII. Since the application can get the resource and free it but the resource will outlive the requester inside this class. But I come from C/C++ and maybe RAII doesn't apply quite well here because the reference counter. I will study about it a little bit. --- 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/crosstest mu...
Github user gadLinux commented on a diff in the pull request: https://github.com/apache/thrift/pull/1200#discussion_r102412630 --- 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 -- ok --- 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/crosstest mu...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1200 --- 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. ---