Re: Testing network availability

[Peter Bloomfield <PeterBloomfield bellsouth net>]

On 05/26/2017 08:14:33 AM Fri, Albrecht Dreß wrote:
> Hi Peter:
>
> Thanks a lot for your feedback!
>
> Am 26.05.17 04:16 schrieb(en) Peter Bloomfield:
> > One thought: in lbs_check_reachable_cb, the GObject *object argument is actually the LibBalsa *server in the 
> > call to libbalsa_server_test_can_reach, so including it in SendQueueInfo is redundant; also,
>
> Yes, you're right.  Good point.
>
> > it's object-reffed in the LibBalsaServer code, so there's no need to object-ref it in lbs_process_queue.
>
> No, it think this is necessary.  Consider the following (though unlikely) situation:
> - user triggers a send operation, which in turn starts the availability check
> - the check takes some time (e.g. a dial-up connection)
> - the user deletes the server definition, which has no effect due to the ref in 
> libbalsa_server_test_can_reach_full()
> - the server is reachable, so libbalsa_server_can_reach_cb() calls the callback which launches the sending 
> thread
> - the server is unred'ef, which will probably lead to a crash in the sending thread.
>
> But actually, the unref is misplaced, and would never catch this case - thanks for pointing me to that!

Hmm, yes…I guess it's wise in any async operation to make sure that any object that you really need in the 
callback is still alive when it's called! It may make some lower level ref/unrefs redundant, but it's surely 
more robust.

> > Passing the server explicitly to lbs_process_queue_real complicates the call a little, but if it is cast as a 
> > (LibBalsaServer *), it would make the first declaration (LibBalsaServer *server = 
> > LIBBALSA_SERVER(send_info->smtp_server)) redundant.
> >
> > What do you think?
>
> Please find attached ver. 2 of the patch, fixing the issues above.
>
> It basically removes the smtp server from SendQueueInfo, but adds it to SendMessageInfo (instead of just its 
> name), so (un) ref'ing it is handled in send_message_info_new() and send_message_info_destroy().

That looks great! About to commit…

Thanks again,

Peter