Bug 37145 - Replace low level network code with GIO
Summary: Replace low level network code with GIO
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: idle (show other bugs)
Version: git master
Hardware: All All
: medium enhancement
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.freedesktop.org/~debarshi...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks: 12376
  Show dependency treegraph
 
Reported: 2011-05-12 09:43 UTC by Debarshi Ray
Modified: 2011-10-04 06:48 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Use guint16 for port numbers and G_MAXUINT16 instead of 0xffff (2.31 KB, patch)
2011-05-14 06:21 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Replace low level network code with GIO (19.01 KB, patch)
2011-05-14 06:33 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Replace low level network code with GIO (20.99 KB, patch)
2011-08-14 07:36 UTC, Debarshi Ray
Details | Splinter Review
Get rid of IdleServerConnectionIface and IdleSSLServerConnection (42.68 KB, patch)
2011-08-14 07:37 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Replace g_signal_emit_by_name with g_signal_emit (1.21 KB, patch)
2011-08-14 07:38 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Reduce the number of certificate checks (1.40 KB, patch)
2011-08-14 07:38 UTC, Debarshi Ray
Details | Splinter Review
Use guint16 for port numbers and G_MAXUINT16 instead of 0xffff (2.31 KB, patch)
2011-10-04 06:31 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Replace low level network code with GIO (21.16 KB, patch)
2011-10-04 06:32 UTC, Debarshi Ray
Details | Splinter Review
Get rid of IdleServerConnectionIface and IdleSSLServerConnection (42.54 KB, patch)
2011-10-04 06:33 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Replace g_signal_emit_by_name with g_signal_emit (1.28 KB, patch)
2011-10-04 06:34 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Reduce the number of certificate checks (1.41 KB, patch)
2011-10-04 06:35 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Async-ify idle_server_connection_connect (9.37 KB, patch)
2011-10-04 06:36 UTC, Debarshi Ray
Details | Splinter Review
IdleConnection: Handle connecting -> disconnected correctly (3.35 KB, patch)
2011-10-04 06:36 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Async-ify idle_server_connection_send (10.55 KB, patch)
2011-10-04 06:37 UTC, Debarshi Ray
Details | Splinter Review
IdleDNSResolver: Not needed anymore (8.74 KB, patch)
2011-10-04 06:38 UTC, Debarshi Ray
Details | Splinter Review
IdleConnection: Use tp_strdiff and tp_str_empty (1.17 KB, patch)
2011-10-04 06:39 UTC, Debarshi Ray
Details | Splinter Review
Consolidate tp_base_connection_disconnect_with_dbus_error calls (4.56 KB, patch)
2011-10-04 06:40 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Async-ify idle_server_connection_disconnect (7.83 KB, patch)
2011-10-04 06:41 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Cleanup usage of the private GCancellable (4.02 KB, patch)
2011-10-04 06:41 UTC, Debarshi Ray
Details | Splinter Review
IdleServerConnection: Simplify the dispose method (1.83 KB, patch)
2011-10-04 06:43 UTC, Debarshi Ray
Details | Splinter Review
IdleConnection: Remove redundant check (1.08 KB, patch)
2011-10-04 06:44 UTC, Debarshi Ray
Details | Splinter Review
IdleConnection: Prevent overlapping and duplicate sends (4.34 KB, patch)
2011-10-04 06:45 UTC, Debarshi Ray
Details | Splinter Review
idletest: The data for an IRC event can be [] too (770 bytes, patch)
2011-10-04 06:46 UTC, Debarshi Ray
Details | Splinter Review
idletest: Use handleCommand instead of dataReceived (1.58 KB, patch)
2011-10-04 06:47 UTC, Debarshi Ray
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Debarshi Ray 2011-05-12 09:43:42 UTC
Idle has its own hand written classes for doing low level network communication -- sockets, name resolution, etc.. We should be using the GIO equivalents of these.
Comment 1 Debarshi Ray 2011-05-14 06:21:35 UTC
Created attachment 46707 [details] [review]
Use guint16 for port numbers and G_MAXUINT16 instead of 0xffff
Comment 2 Debarshi Ray 2011-05-14 06:33:59 UTC
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.
Comment 3 Debarshi Ray 2011-05-14 06:35:48 UTC
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.
Comment 4 Debarshi Ray 2011-05-14 13:45:24 UTC
You can find these patches in my GIO branch. See URL.
Comment 5 Sjoerd Simons 2011-05-30 07:50:22 UTC
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.. :)
Comment 6 Debarshi Ray 2011-08-07 08:29:10 UTC
(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?
Comment 7 Debarshi Ray 2011-08-09 10:00:44 UTC
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.
Comment 8 Debarshi Ray 2011-08-10 01:42:33 UTC
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.
Comment 9 Sjoerd Simons 2011-08-10 05:29:35 UTC
* 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).
Comment 10 Debarshi Ray 2011-08-10 06:52:59 UTC
(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?
Comment 11 Sjoerd Simons 2011-08-10 07:12:01 UTC
(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
Comment 12 Debarshi Ray 2011-08-14 07:35:24 UTC
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.
Comment 13 Debarshi Ray 2011-08-14 07:36:27 UTC
Created attachment 50200 [details] [review]
IdleServerConnection: Replace low level network code with GIO
Comment 14 Debarshi Ray 2011-08-14 07:37:14 UTC
Created attachment 50201 [details] [review]
Get rid of IdleServerConnectionIface and IdleSSLServerConnection
Comment 15 Debarshi Ray 2011-08-14 07:38:07 UTC
Created attachment 50202 [details] [review]
IdleServerConnection: Replace g_signal_emit_by_name with g_signal_emit
Comment 16 Debarshi Ray 2011-08-14 07:38:50 UTC
Created attachment 50203 [details] [review]
IdleServerConnection: Reduce the number of certificate checks
Comment 17 Debarshi Ray 2011-08-17 13:28:26 UTC
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.
Comment 18 Will Thompson 2011-09-09 07:38:29 UTC
+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")
Comment 19 Debarshi Ray 2011-09-10 14:27:17 UTC
(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?
Comment 20 Will Thompson 2011-09-15 09:40:27 UTC
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.
Comment 21 Debarshi Ray 2011-09-16 11:08:20 UTC
> +    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.
Comment 22 Debarshi Ray 2011-09-18 12:21:07 UTC
> > 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.
Comment 23 Debarshi Ray 2011-09-18 16:10:51 UTC
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.
Comment 24 Debarshi Ray 2011-09-29 15:19:59 UTC
Rebased against master.
Comment 25 Will Thompson 2011-10-03 10:43:26 UTC
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!
Comment 26 Debarshi Ray 2011-10-04 06:02:48 UTC
(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.
Comment 27 Debarshi Ray 2011-10-04 06:31:16 UTC
Created attachment 51938 [details] [review]
Use guint16 for port numbers and G_MAXUINT16 instead of 0xffff
Comment 28 Debarshi Ray 2011-10-04 06:32:11 UTC
Created attachment 51939 [details] [review]
IdleServerConnection: Replace low level network code with GIO
Comment 29 Debarshi Ray 2011-10-04 06:33:13 UTC
Created attachment 51940 [details] [review]
Get rid of IdleServerConnectionIface and IdleSSLServerConnection
Comment 30 Debarshi Ray 2011-10-04 06:34:09 UTC
Created attachment 51941 [details] [review]
IdleServerConnection: Replace g_signal_emit_by_name with g_signal_emit
Comment 31 Debarshi Ray 2011-10-04 06:35:04 UTC
Created attachment 51942 [details] [review]
IdleServerConnection: Reduce the number of certificate checks
Comment 32 Debarshi Ray 2011-10-04 06:36:15 UTC
Created attachment 51943 [details] [review]
IdleServerConnection: Async-ify idle_server_connection_connect
Comment 33 Debarshi Ray 2011-10-04 06:36:57 UTC
Created attachment 51944 [details] [review]
IdleConnection: Handle connecting -> disconnected correctly
Comment 34 Debarshi Ray 2011-10-04 06:37:36 UTC
Created attachment 51945 [details] [review]
IdleServerConnection: Async-ify idle_server_connection_send
Comment 35 Debarshi Ray 2011-10-04 06:38:51 UTC
Created attachment 51946 [details] [review]
IdleDNSResolver: Not needed anymore
Comment 36 Debarshi Ray 2011-10-04 06:39:41 UTC
Created attachment 51947 [details] [review]
IdleConnection: Use tp_strdiff and tp_str_empty
Comment 37 Debarshi Ray 2011-10-04 06:40:26 UTC
Created attachment 51948 [details] [review]
Consolidate tp_base_connection_disconnect_with_dbus_error calls
Comment 38 Debarshi Ray 2011-10-04 06:41:06 UTC
Created attachment 51949 [details] [review]
IdleServerConnection: Async-ify idle_server_connection_disconnect
Comment 39 Debarshi Ray 2011-10-04 06:41:59 UTC
Created attachment 51950 [details] [review]
IdleServerConnection: Cleanup usage of the private GCancellable
Comment 40 Debarshi Ray 2011-10-04 06:43:40 UTC
Created attachment 51951 [details] [review]
IdleServerConnection: Simplify the dispose method
Comment 41 Debarshi Ray 2011-10-04 06:44:42 UTC
Created attachment 51952 [details] [review]
IdleConnection: Remove redundant check
Comment 42 Debarshi Ray 2011-10-04 06:45:39 UTC
Created attachment 51953 [details] [review]
IdleConnection: Prevent overlapping and duplicate sends
Comment 43 Debarshi Ray 2011-10-04 06:46:31 UTC
Created attachment 51954 [details] [review]
idletest: The data for an IRC event can be [] too
Comment 44 Debarshi Ray 2011-10-04 06:47:23 UTC
Created attachment 51955 [details] [review]
idletest: Use handleCommand instead of dataReceived
Comment 45 Debarshi Ray 2011-10-04 06:48:28 UTC
Merged into master and pushed.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.