Bug 37145

Created attachment 46707 [details] [review]
Use guint16 for port numbers and G_MAXUINT16 instead of 0xffff

Created attachment 46708 [details] [review]
IdleServerConnection: Replace low level network code with GIO

Known problems:

1. When iface_disconnect_impl_full is called we cancel priv->cancellable to remove the GSourceFunc. This happens by triggering _socket_received (ie. the GSourceFunc) one more time and we return FALSE if priv->cancellable is cancelled. According to the documentation for g_socket_create_source the value of condition  "is likely 0 unless cancellation happened at the same time as a condition change". So there is a possibility that some data that arrived immediately before the disconnection will be lost.

    Avoiding this is a bit tricky because iface_disconnect_impl_full can be called from the dispose method as well and to return the data it emits a signal on the IdleServerConnection object, which in this case is the object being disposed.

2. iface_disconnect_impl_full does not cancel connections in the "connecting" state. It is just a matter of cancelling the priv->cancellable but I did not want to change the behaviour of the older code.

3. I am not sure why the 'g_assert(priv != NULL)' was been used in the earlier code.

The next step would be to rip off the IdleServerConnectionIface because we no longer need to provide multiple implementations to support plain-text, SSL, etc. connections. GIO does that for us.

You can find these patches in my GIO branch. See URL.

My first pass, sorry, they're verry rough...

* 49871d7df27a8fa3964b87fe39831d9652431eda, looks good

* _socket_source_data_destroy -> doesn't free the cancellable ?

* g_idle_add(_object_destroy_idle, priv->cancellable); 
  destroying in an idle that seems suspicious?

* At dispose time all your outstanding operation really should have finished

* g_idle_add(io_err_cleanup_func, conn); idle destroys again ?

*  _socket_received -> Why ? why not just call _read_async?

* connect_to_host, leaks &error (and worse, ignores it) on error conditions

* Why using GSockets sources by hand ? Don't, instead use the async read
    functions.

* Things we lost: Setting SO_KEEPALIVE, Setting TCP_NODELAY

=> disconnect, cancel and then closing the connection ?
  -> Not Reporting an error on disconnect ?

* iface_send_impl is using a blocking write. it shouldn't.. :)

(In reply to comment #5)

I have update my branch. Could you please take another look at it?

> * g_idle_add(io_err_cleanup_func, conn); idle destroys again ?

This is a relic from the previous code. The reason I have still kept it is that the _read_async was called on the input_stream which is a part of the priv->socket_connection object and the disconnect method unrefs this same object. I am not sure how that will play out if I don't leave the main loop.

> * iface_send_impl is using a blocking write. it shouldn't.. :)

Another relic from the past. This is closely related to my above question. In short, if I do an asynchronous write and detect a problem in the callback, then can I disconnect without without leaving the mainloop?

Another update.

Now I will get rid of the IdleServerConnectionIface because we no longer need two separate implementations. My understanding is that we can use g_socket_client_set_tls on a single IdleServerConnection. Do you think it would be a good idea to make IdleServerConnection a subclass of GSocketClient?

While we are at it, I am also going to modify the interface to make it more async-friendly. The current one is designed for synchronous calls.

Another update.

Both write and close are now asynchronous, and I have taken a stab at getting rid of the IdleServerConnectionIface and IdleSSLServerConnection classes. However, as a result of that I am having trouble with SSL connections.

* Don't use g_signal_emit_by_name, use g_signal_emit direct

* The following breaks SSL:
   priv->io_stream = g_tcp_wrapper_connection_get_base_io_stream(
        G_TCP_WRAPPER_CONNECTION(socket_connection));

* CloseAsyncData and WriteAsyncData seem mostly useless. As both operations only should run once per 
   connection object just keep the state in the priv data structure (shoulod simlify the code).

(In reply to comment #9)
> * Don't use g_signal_emit_by_name, use g_signal_emit direct

Fixed.
 
> * The following breaks SSL:
>    priv->io_stream = g_tcp_wrapper_connection_get_base_io_stream(
>         G_TCP_WRAPPER_CONNECTION(socket_connection));

Fixed. But the connect/connect-success-ssl.py test is failing, although I can connect to Freenode. Wondering if this has something to do with self-signed certificates.
 
> * CloseAsyncData and WriteAsyncData seem mostly useless. As both operations
> only should run once per 
>    connection object just keep the state in the priv data structure (shoulod
> simlify the code).

What if the send interface is invoked twice in succession before the first one has completed? Then the contexts would get mixed up, won't they?

(In reply to comment #10)
> > * The following breaks SSL:
> >    priv->io_stream = g_tcp_wrapper_connection_get_base_io_stream(
> >         G_TCP_WRAPPER_CONNECTION(socket_connection));
> 
> Fixed. But the connect/connect-success-ssl.py test is failing, although I can
> connect to Freenode. Wondering if this has something to do with self-signed
> certificates.

It does, gtls does quite strict checking  by default. We should for-now switch back to check at the same level as the old code and for the future implement SSL connection channel

> > * CloseAsyncData and WriteAsyncData seem mostly useless. As both operations
> > only should run once per 
> >    connection object just keep the state in the priv data structure (shoulod
> > simlify the code).
> 
> What if the send interface is invoked twice in succession before the first one
> has completed? Then the contexts would get mixed up, won't they?

That case is already broken (didn't catch it before, sorry), you can't call _write_async on a GOutputStream when the previous async operation hasn't finished yet. To implement that kind of API you need to implement some buffering yourself

I have updated my branch.

+ The certificate validation has now been relaxed so that the test suite passes cleanly.
+ Got rid of CloseAsyncData and WriteAsyncData.

Created attachment 50200 [details] [review]
IdleServerConnection: Replace low level network code with GIO

Created attachment 50201 [details] [review]
Get rid of IdleServerConnectionIface and IdleSSLServerConnection

Created attachment 50202 [details] [review]
IdleServerConnection: Replace g_signal_emit_by_name with g_signal_emit

Created attachment 50203 [details] [review]
IdleServerConnection: Reduce the number of certificate checks

I have pushed some more patches to my branch.

I am trying to make the interface of IdleServerConnection more "asynchronous friendly" like GIO's _async/_finish methods. Currently we had a strange mix of asynchronous and synchronous operations going behind interfaces designed for synchronous usage.

+typedef struct _CloseAsyncData CloseAsyncData;
+typedef struct _WriteAsyncData WriteAsyncData;

Vestigal typedefs!

+	if (g_input_stream_read_finish(input_stream, res, &error) == -1) {
+		IDLE_DEBUG("g_input_stream_read failed: %s", error->message);
+		g_error_free(error);
+		goto disconnect;
 	}

+	if (socket_connection == NULL) {
+		IDLE_DEBUG("g_socket_client_connect_to_host failed: %s", error->message);
+		g_error_free(error);
+		change_state(conn, SERVER_CONNECTION_STATE_NOT_CONNECTED, SERVER_CONNECTION_STATE_REASON_ERROR);

It's a shame that we lose the errors in these cases. Ah, I see that in the second case, you later hooked up the message.

+	memset(priv->input_buffer, '\0', sizeof(priv->input_buffer));
+	g_input_stream_read_async (input_stream, &priv->input_buffer, sizeof(priv->input_buffer) - 1, G_PRIORITY_DEFAULT, priv->cancellable, _input_stream_read_ready, conn);

This is duplicated, it'd be nice to have a helper function for this.

+	g_cancellable_reset(priv->cancellable);
+	g_object_ref(conn);
+	g_socket_client_connect_to_host_async(priv->socket_client, priv->host, priv->port, priv->cancellable, _connect_to_host_ready, conn);

I don't see why you're resetting the cancellable here. If it was in the cancelled state, surely we shouldn't be trying to connect at all.

  IdleConnection: Initialize priv->conn

+	priv->conn = NULL;

This shouldn't be needed: GObject zeroes out your structures when it allocates them.

 static void _connect_to_host_ready(GObject *source_object, GAsyncResult *res, gpointer user_data) {
 	GSocketClient *socket_client = G_SOCKET_CLIENT(source_object);
-	IdleServerConnection *conn = IDLE_SERVER_CONNECTION(user_data);
+	GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(user_data);
+	IdleServerConnection *conn = IDLE_SERVER_CONNECTION(g_async_result_get_source_object(G_ASYNC_RESULT(result)));
…
 		change_state(conn, SERVER_CONNECTION_STATE_NOT_CONNECTED, SERVER_CONNECTION_STATE_REASON_ERROR);
-		g_object_unref(conn);
-		return;
+		goto cleanup;
…
+	g_cancellable_reset(priv->cancellable);
+	g_object_ref(conn);
 	g_input_stream_read_async (input_stream, &priv->input_buffer, sizeof(priv->input_buffer) - 1, G_PRIORITY_DEFAULT, priv->cancellable, _input_stream_read_ready, conn);

g_async_result_get_source_object() returns a reference, so if I'm not mistaken, the removal of the call to g_object_unref and the addition of the call to g_object_ref mean you will leak a reference.

IdleConnection: No need to disconnect if connection can not be made

I don't understand this change: if _iface_shut_down is called while we're in state CONNECTING, surely we do want to tell the IdleServerConnection to give up?

These are in existing code that you just moved around, feel free to ignore them:

+	if (!priv->realname || !priv->realname[0]) {

tp_str_empty()

+		if (g_realname && g_realname[0] && strcmp(g_realname, "Unknown"))

!tp_strdiff (g_realname, "Unknown")

(In reply to comment #18)
> +typedef struct _CloseAsyncData CloseAsyncData;
> +typedef struct _WriteAsyncData WriteAsyncData;
> 
> Vestigal typedefs!

Removed.
 
> +    if (g_input_stream_read_finish(input_stream, res, &error) == -1) {
> +        IDLE_DEBUG("g_input_stream_read failed: %s", error->message);
> +        g_error_free(error);
> +        goto disconnect;
>      }
> [...]
>
> It's a shame that we lose the errors in these cases. Ah, I see that in the
> second case, you later hooked up the message.

Should we pass the error through the signal?
 
> +    memset(priv->input_buffer, '\0', sizeof(priv->input_buffer));
> +    g_input_stream_read_async (input_stream, &priv->input_buffer,
> sizeof(priv->input_buffer) - 1, G_PRIORITY_DEFAULT, priv->cancellable,
> _input_stream_read_ready, conn);
> 
> This is duplicated, it'd be nice to have a helper function for this.

Will do.

> +    g_cancellable_reset(priv->cancellable);
> +    g_object_ref(conn);
> +    g_socket_client_connect_to_host_async(priv->socket_client, priv->host,
> priv->port, priv->cancellable, _connect_to_host_ready, conn);
> 
> I don't see why you're resetting the cancellable here. If it was in the
> cancelled state, surely we shouldn't be trying to connect at all.

I think we can get rid of the GCancellable inside IdleServerConnection once we have an _async version of the disconnect method.

>   IdleConnection: Initialize priv->conn
> 
> +    priv->conn = NULL;
> 
> This shouldn't be needed: GObject zeroes out your structures when it allocates
> them.

Removed. Also removed similar instance of 'priv->io_stream = NULL' from idle_server_connection_init.

>  static void _connect_to_host_ready(GObject *source_object, GAsyncResult *res,
> gpointer user_data) {
>      GSocketClient *socket_client = G_SOCKET_CLIENT(source_object);
> -    IdleServerConnection *conn = IDLE_SERVER_CONNECTION(user_data);
> +    GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(user_data);
> +    IdleServerConnection *conn =
> IDLE_SERVER_CONNECTION(g_async_result_get_source_object(G_ASYNC_RESULT(result)));
> …
>          change_state(conn, SERVER_CONNECTION_STATE_NOT_CONNECTED,
> SERVER_CONNECTION_STATE_REASON_ERROR);
> -        g_object_unref(conn);
> -        return;
> +        goto cleanup;
> …
> +    g_cancellable_reset(priv->cancellable);
> +    g_object_ref(conn);
>      g_input_stream_read_async (input_stream, &priv->input_buffer,
> sizeof(priv->input_buffer) - 1, G_PRIORITY_DEFAULT, priv->cancellable,
> _input_stream_read_ready, conn);
> 
> g_async_result_get_source_object() returns a reference, so if I'm not mistaken,
> the removal of the call to g_object_unref and the addition of the call to
> g_object_ref mean you will leak a reference.

You are right. I though 'get' methods don't return a reference. I have tried to fix the leak by restoring the g_object_unref and removing the extra g_object_ref.

> IdleConnection: No need to disconnect if connection can not be made
> 
> I don't understand this change: if _iface_shut_down is called while we're in
> state CONNECTING, surely we do want to tell the IdleServerConnection to give
> up?

I have improved this patch with a better explanation in the commit message and comments. See this commit:
IdleConnection: Handle connecting -> disconnected correctly

If you take this out, then the connect/connect-fail.py and connect/connect-fail-ssl.py tests will fail due a segmentation fault.

> These are in existing code that you just moved around, feel free to ignore
> them:
> 
> +    if (!priv->realname || !priv->realname[0]) {
> 
> tp_str_empty()

Will do.

> +        if (g_realname && g_realname[0] && strcmp(g_realname, "Unknown"))
> 
> !tp_strdiff (g_realname, "Unknown")

What about g_strcmp0?

I notice that we've still lost setting TCP_NODELAY.

(In reply to comment #19)
> (In reply to comment #18)
> > +    if (g_input_stream_read_finish(input_stream, res, &error) == -1) {
> > +        IDLE_DEBUG("g_input_stream_read failed: %s", error->message);
> > +        g_error_free(error);
> > +        goto disconnect;
> >      }
> > [...]
> >
> > It's a shame that we lose the errors in these cases. Ah, I see that in the
> > second case, you later hooked up the message.
> 
> Should we pass the error through the signal?

It might be nice—I don't think it's essential, though. At most it will end up in debug-message in the ConnectionErrorDetails.

> > +    g_cancellable_reset(priv->cancellable);
> > +    g_object_ref(conn);
> > +    g_socket_client_connect_to_host_async(priv->socket_client, priv->host,
> > priv->port, priv->cancellable, _connect_to_host_ready, conn);
> > 
> > I don't see why you're resetting the cancellable here. If it was in the
> > cancelled state, surely we shouldn't be trying to connect at all.
> 
> I think we can get rid of the GCancellable inside IdleServerConnection once we
> have an _async version of the disconnect method.

Maybe so, but I still don't understand why you're calling _reset here. :)

> >  static void _connect_to_host_ready(GObject *source_object, GAsyncResult *res,
> > gpointer user_data) {
> >      GSocketClient *socket_client = G_SOCKET_CLIENT(source_object);
> > -    IdleServerConnection *conn = IDLE_SERVER_CONNECTION(user_data);
> > +    GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(user_data);
> > +    IdleServerConnection *conn =
> > IDLE_SERVER_CONNECTION(g_async_result_get_source_object(G_ASYNC_RESULT(result)));
> > …
> >          change_state(conn, SERVER_CONNECTION_STATE_NOT_CONNECTED,
> > SERVER_CONNECTION_STATE_REASON_ERROR);
> > -        g_object_unref(conn);
> > -        return;
> > +        goto cleanup;
> > …
> > +    g_cancellable_reset(priv->cancellable);
> > +    g_object_ref(conn);
> >      g_input_stream_read_async (input_stream, &priv->input_buffer,
> > sizeof(priv->input_buffer) - 1, G_PRIORITY_DEFAULT, priv->cancellable,
> > _input_stream_read_ready, conn);
> > 
> > g_async_result_get_source_object() returns a reference, so if I'm not mistaken,
> > the removal of the call to g_object_unref and the addition of the call to
> > g_object_ref mean you will leak a reference.
> 
> You are right. I though 'get' methods don't return a reference. I have tried to
> fix the leak by restoring the g_object_unref and removing the extra
> g_object_ref.

Looks right now.

> > IdleConnection: No need to disconnect if connection can not be made
> > 
> > I don't understand this change: if _iface_shut_down is called while we're in
> > state CONNECTING, surely we do want to tell the IdleServerConnection to give
> > up?
> 
> I have improved this patch with a better explanation in the commit message and
> comments. See this commit:
> IdleConnection: Handle connecting -> disconnected correctly
> 
> If you take this out, then the connect/connect-fail.py and
> connect/connect-fail-ssl.py tests will fail due a segmentation fault.

Okay, this makes a little more sense to me now. :)

> > These are in existing code that you just moved around, feel free to ignore
> > them:
> > 
> > +        if (g_realname && g_realname[0] && strcmp(g_realname, "Unknown"))
> > 
> > !tp_strdiff (g_realname, "Unknown")
> 
> What about g_strcmp0?

I don't like g_strcmp0 because it throws away type information. (It really depresses me that there are so many different ways to compare strings in this language…)

FWIW, having two commits titled «IdleServerConnection: Make the interface "asynchronous friendly"» whose bodies say what they actually do «Replace idle_server_connection_connect with
idle_server_connection_connect_async and
idle_server_connection_connect_finish.» makes for slightly confusing reading. (Rather than, say, “Async-ify idle_server_connection_connect” and “Async-ify idle_server_connection_send”.)


+	if (!idle_server_connection_connect_finish(sconn, res, &error)) {
+		IDLE_DEBUG("idle_server_connection_connect failed: %s", error->message);
 
-		tp_base_connection_disconnect_with_dbus_error(base_conn,
-							      TP_ERROR_STR_NETWORK_ERROR,
+		tp_base_connection_disconnect_with_dbus_error(TP_BASE_CONNECTION(conn),
+							      tp_error_get_dbus_name(error->code),
 							      NULL,
 							      TP_CONNECTION_STATUS_REASON_NETWORK_ERROR);
 
+		g_error_free(error);

You should probably sanity-check that error->domain == TP_ERRORS.

If you pass a hash table built using something like tp_asv_new ("debug-message", G_TYPE_STRING, error->message, NULL); as the third argument to disconnect_with_dbus_error, this could conceivably be shown by a UI in some kind of “technical details” expander.

> +    memset(priv->input_buffer, '\0', sizeof(priv->input_buffer));
> +    g_input_stream_read_async (input_stream, &priv->input_buffer,
> sizeof(priv->input_buffer) - 1, G_PRIORITY_DEFAULT, priv->cancellable,
> _input_stream_read_ready, conn);
> 
> This is duplicated, it'd be nice to have a helper function for this.

Done.

> I notice that we've still lost setting TCP_NODELAY.

Using setsockopt since I could not locate a GSocket API. Worth adding one to Glib?

> (In reply to comment #19)
>> (In reply to comment #18)
>>> +    if (g_input_stream_read_finish(input_stream, res, &error) == -1) {
>>> +        IDLE_DEBUG("g_input_stream_read failed: %s", error->message);
>>> +        g_error_free(error);
>>> +        goto disconnect;
>>>      }
>>> [...]
>>>
>>> It's a shame that we lose the errors in these cases. Ah, I see that in the
>>> second case, you later hooked up the message.
>> 
>> Should we pass the error through the signal?
> 
> It might be nice—I don't think it's essential, though. At most it will end up
> in debug-message in the ConnectionErrorDetails.

Ok. I have left it as it was.

>>> These are in existing code that you just moved around, feel free to ignore
>>> them:

Replaced with tp_strdiff & tp_str_empty.

> FWIW, having two commits titled «IdleServerConnection: Make the interface
> "asynchronous friendly"» whose bodies say what they actually do «Replace
> idle_server_connection_connect with
> idle_server_connection_connect_async and
> idle_server_connection_connect_finish.» makes for slightly confusing reading.
> (Rather than, say, “Async-ify idle_server_connection_connect” and “Async-ify
> idle_server_connection_send”.)

Done.

> +    if (!idle_server_connection_connect_finish(sconn, res, &error)) {
> +        IDLE_DEBUG("idle_server_connection_connect failed: %s",
> error->message);
> 
> -        tp_base_connection_disconnect_with_dbus_error(base_conn,
> -                                  TP_ERROR_STR_NETWORK_ERROR,
> +       
> tp_base_connection_disconnect_with_dbus_error(TP_BASE_CONNECTION(conn),
> +                                  tp_error_get_dbus_name(error->code),
>                                    NULL,
>                                    TP_CONNECTION_STATUS_REASON_NETWORK_ERROR);
> 
> +        g_error_free(error);
> 
> You should probably sanity-check that error->domain == TP_ERRORS.
> 
> If you pass a hash table built using something like tp_asv_new
> ("debug-message", G_TYPE_STRING, error->message, NULL); as the third argument
> to disconnect_with_dbus_error, this could conceivably be shown by a UI in some
> kind of “technical details” expander.

Will do.

> > You should probably sanity-check that error->domain == TP_ERRORS.
> > 
> > If you pass a hash table built using something like tp_asv_new
> > ("debug-message", G_TYPE_STRING, error->message, NULL); as the third argument
> > to disconnect_with_dbus_error, this could conceivably be shown by a UI in some
> > kind of “technical details” expander.
> 
> Will do.

Done.

I just pushed a few more patches into my tree. I think I have covered all that I set out to do. So all that remains now is to get this final set reviewed and then we can merge into master.

Please note that I fixed a reference counting issue in the _write_ready function in src/idle-server-connection.c where the IdleServerConnection instance was getting leaked. I hope I got it right this time.

Rebased against master.

Re “idletest: Split the data if it has multiple messages before parsing it”, I think it will work, but… Looking at <http://twistedmatrix.com/documents/10.0.0/api/twisted.words.protocols.irc.IRC.html#dataReceived> and the method below it, I believe we could replace the test suite manually splitting up lines and messages, by implementing handleCommand rather than dataReceived.

Not a merge blocker though.

I'm not entirely sure that we need to re-queue messages if idle_server_connection_send_finish() fails—surely that only happens if the underlying socket connection dies, in which case the IRC session is over anyway? (In the current Tp model, that is.) But the code doesn't look *wrong*, so let's leave it.

Hooray!

(In reply to comment #25)
> Re “idletest: Split the data if it has multiple messages before parsing it”, I
> think it will work, but… Looking at
> <http://twistedmatrix.com/documents/10.0.0/api/twisted.words.protocols.irc.IRC.html#dataReceived>
> and the method below it, I believe we could replace the test suite manually
> splitting up lines and messages, by implementing handleCommand rather than
> dataReceived.

Using handleCommand now.

Created attachment 51938 [details] [review]
Use guint16 for port numbers and G_MAXUINT16 instead of 0xffff

Created attachment 51939 [details] [review]
IdleServerConnection: Replace low level network code with GIO

Created attachment 51940 [details] [review]
Get rid of IdleServerConnectionIface and IdleSSLServerConnection

Created attachment 51941 [details] [review]
IdleServerConnection: Replace g_signal_emit_by_name with g_signal_emit

Created attachment 51942 [details] [review]
IdleServerConnection: Reduce the number of certificate checks

Created attachment 51943 [details] [review]
IdleServerConnection: Async-ify idle_server_connection_connect

Created attachment 51944 [details] [review]
IdleConnection: Handle connecting -> disconnected correctly

Created attachment 51945 [details] [review]
IdleServerConnection: Async-ify idle_server_connection_send

Created attachment 51946 [details] [review]
IdleDNSResolver: Not needed anymore

Created attachment 51947 [details] [review]
IdleConnection: Use tp_strdiff and tp_str_empty

Created attachment 51948 [details] [review]
Consolidate tp_base_connection_disconnect_with_dbus_error calls

Created attachment 51949 [details] [review]
IdleServerConnection: Async-ify idle_server_connection_disconnect

Created attachment 51950 [details] [review]
IdleServerConnection: Cleanup usage of the private GCancellable

Created attachment 51951 [details] [review]
IdleServerConnection: Simplify the dispose method

Created attachment 51952 [details] [review]
IdleConnection: Remove redundant check

Created attachment 51953 [details] [review]
IdleConnection: Prevent overlapping and duplicate sends

Created attachment 51954 [details] [review]
idletest: The data for an IRC event can be [] too

Created attachment 51955 [details] [review]
idletest: Use handleCommand instead of dataReceived

Merged into master and pushed.