Re: [gnet-dev] Asynchronous DNS query when nonblocking query is possible

[cascardo minaslivre org]

Sorry. Did not reply to the list, but only to Tim. Sorry, Tim.


On Thu, Aug 31, 2006 at 04:10:10PM +0100, Tim M?ller wrote:
> On Thu, 2006-08-31 at 10:25 -0300, cascardo minaslivre org wrote:
> 
> Hi,
> 
> > One year later I sent a message regarding using uDNS in GNET, I'm really
> > writing it. I would really like to finish it and get it included in
> > GNET.
> 
> To be honest, I'm not really sure whether it's a good idea to add
> further library dependencies to GNet, even more so a dependency on a
> somewhat 'exotic' library like libudns that isn't widely packaged. If
> someone requires the performance/robustness of such specialised
> libraries, they should be using that library directly IMHO (not that
> improvements to GNet aren't welcome, it's just that GNet is targeted at
> people with different needs).
> 

I was thinking in making it optional, with a --with-udns option in
autoconf. Besides, udns is small, about 5k lines of code, including
ability to parse TXT, MX, RR and other registers. Only inetaddr.c is
about 3k lines. It would be reasonable to include udns in the
distribution, IMHO.

> 
> > The first thing I am working on is getting nonblocking queries (those
> > with a decimal dot notation address instead of a real DNS name) not to
> > dispatch a new thread or process.
> > 
> > However, that would require calling the callback function before even
> > returning the new_async functions. Would that be a problem?
> 
> You can't really do that (calling the callback function before returning
> from new_async) I'm afraid. The documentation explicitly states that the
> callback will not be called before the function returns. Changing that
> would mean breaking backwards compatibility.
> 

Agreed.

> However, if the fork/thread creation overhead is your main concern, what
> would be possible is to try gnet_inetaddr_new_nonblock() or similar
> first in _new_async() and if successful set up an dispatcher idle
> callback right in _new_async() without creating the thread or forking.
> That saves you the fork/thread overhead and maintains the current API
> semantics. 
> 

Fine. Just didn't think about using g_idle_add before.

> > Which AsyncID should I return? I've noticed that AsyncID is, in fact, a
> > pointer to a State, and it seems applications are responsible not to
> > call cancel when the callback function was already called (when state
> > would be freed). So, returning a NULL ID would be the right way, since
> > cancel would also return without failure even in case the application
> > called cancel.
> 
> You must return some kind of non-NULL AsyncID, since returning NULL
> indicates failure according to the API documentation.
> 
> (Alternatively, you could introduce new API of course, but you would
> need to have a reasonably good argument for why that's needed).
> 

Since I would be dispatching the callback from an idle dispatcher, the
same state usually saved in AsyncID would be needed. Then, no new API is
needed, since the ID returned is not NULL.

> Cheers
>  -Tim
> 

Attached is the patch licensed under LGPL, modified at 31 Aug 2006 by
Thadeu Lima de Souza Cascardo. It does not make any use of udns. It only
makes it easier to use it and does not fork or create a new thread when
resolving the name is nonblocking.

If a ChangeLog entry is required, I'll send it right away. The
inetaddr_test passes, but it does not test asynchronous interface. I
will make some tests here and report.

Regards,
Thadeu Cascardo.

Index: src/inetaddr.c
===================================================================
RCS file: /cvs/gnome/gnet/src/inetaddr.c,v
retrieving revision 1.89
diff -u -r1.89 inetaddr.c
--- src/inetaddr.c	12 Aug 2006 10:31:50 -0000	1.89
+++ src/inetaddr.c	31 Aug 2006 17:02:33 -0000
@@ -168,50 +168,6 @@
   GList* list = NULL;
 
   /* **************************************** */
-  /* First attempt non-blocking lookup */
-  
-  struct in_addr inaddr;
-#ifdef HAVE_IPV6
-  struct in6_addr in6addr;
-#endif
-
-  if (inet_pton(AF_INET, hostname, &inaddr) > 0)
-    {
-      GInetAddr* ia;
-      struct sockaddr_in* sa;
-
-      ia = g_new0(GInetAddr, 1);
-      ia->ref_count = 1;
-      sa = (struct sockaddr_in*) &ia->sa;
-
-      sa->sin_family = AF_INET;
-      GNET_INETADDR_SET_SS_LEN(ia);
-      sa->sin_addr = inaddr;
-
-      list = g_list_prepend (list, ia);
-      return list;
-    }
-#ifdef HAVE_IPV6
-  else if (inet_pton(AF_INET6, hostname, &in6addr) > 0)
-    {
-      GInetAddr* ia;
-      struct sockaddr_in6* sa;
-
-      ia = g_new0(GInetAddr, 1);
-      ia->ref_count = 1;
-      sa = (struct sockaddr_in6*) &ia->sa;
-
-      sa->sin6_family = AF_INET6;
-      GNET_INETADDR_SET_SS_LEN(ia);
-      sa->sin6_addr = in6addr;
-      /* don't set the nice name, it's not necessarily canonical */
-
-      list = g_list_prepend (list, ia);
-      return list;
-    }
-#endif
-
-  /* **************************************** */
   /* DNS lookup: getaddrinfo() */
 
 #ifdef HAVE_GETADDRINFO
@@ -605,6 +561,13 @@
   GList* ialist;
   GInetAddr* ia = NULL;
 
+  /*
+   * First attempt nonblocking lookup
+   */
+  ia = gnet_inetaddr_new_nonblock (hostname, port);
+  if (ia)
+    return ia;
+
   ialist = gnet_gethostbyname (hostname);
   if (!ialist)
     return NULL;
@@ -636,9 +599,17 @@
 {
   GList* ialist;
   GList* i;
+  GInetAddr* ia;
 
   g_return_val_if_fail (hostname != NULL, NULL);
 
+  /*
+   * First attempt nonblocking lookup
+   */
+  ia = gnet_inetaddr_new_nonblock (hostname, port);
+  if (ia)
+    return g_list_prepend (NULL, ia);
+
   /* Try to get the host by name (ie, DNS) */
   ialist = gnet_gethostbyname(hostname);
   if (!ialist)
@@ -797,6 +768,7 @@
 /* **************************************** */
 /* gnet_inetaddr_new_list_async()	    */
 
+static gboolean inetaddr_new_list_async_nonblock_dispatch (gpointer data);
 
 #ifdef G_THREADS_ENABLED
 static void* inetaddr_new_list_async_gthread (void* arg);
@@ -873,10 +845,29 @@
 			      gpointer data)
 {
   GInetAddrNewListState* state = NULL;
+  GInetAddr* ia;
 
   g_return_val_if_fail(hostname != NULL, NULL);
   g_return_val_if_fail(func != NULL, NULL);
 
+  /*
+   * First attempt nonblocking lookup
+   */
+  ia = gnet_inetaddr_new_nonblock (hostname, port);
+  if (ia)
+    {
+      state = g_new0 (GInetAddrNewListState, 1);
+#ifdef G_THREADS_ENABLED
+      g_static_mutex_init (&state->mutex);
+      g_static_mutex_lock (&state->mutex);
+#endif
+      state->ias = g_list_prepend (NULL, ia);
+      state->source = g_idle_add_full (G_PRIORITY_DEFAULT,
+				       inetaddr_new_list_async_nonblock_dispatch,
+				       state, NULL);
+      return state;
+    }
+  else
 # ifdef G_THREADS_ENABLED
   {
     GThread *thread;
@@ -1030,6 +1021,30 @@
 # endif
 
   return state;
+}
+
+gboolean inetaddr_new_list_async_nonblock_dispatch (gpointer data)
+{
+  GInetAddrNewListState* state = (GInetAddrNewListState*) data;
+
+#ifdef G_THREAD_ENABLED
+  g_thread_mutex_lock (&state->mutex);
+#endif
+
+  state->in_callback = TRUE;
+
+  (*state->func) (state->ias, state->data);
+
+  state->in_callback = FALSE;
+
+  g_source_remove (state->source);
+#ifdef G_THREADS_ENABLES
+  g_thread_mutex_unlock (&state->mutex);
+  g_thread_mutex_free (&state->mutex);
+#endif
+  g_free (state);
+
+  return FALSE;
 }
 
 #ifdef G_THREADS_ENABLED