Testing network availability

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Testing network availability

Peter Bloomfield
Dear List,

The git "master" branch has some new code, for more careful checking of when a POP3 host can be reached. This should avert some warnings about failure to connect to POP3 hosts. Please report any issues with it!

Best regards,

Peter
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list

attachment0 (188 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Albrecht Dreß
Hi all!

Am 23.05.17 00:58 schrieb(en) Peter Bloomfield:
> The git "master" branch has some new code, for more careful checking of when a POP3 host can be reached. This should avert some warnings about failure to connect to POP3 hosts. Please report any issues with it!

As a follow-up to this patch, I implemented the same feature for outgoing smtp connections, using Peter's new functions.

Basically, when a specific smtp server is not reachable, a warning is shown, and the messages are left in the outbox.  Unlike after an error, the messages are /not/ tagged.  Therefore, simply choosing "Send queued mail" will send the messages if possible, or just leave them (untagged) in the outbox if the server is still unreachable.  I did not implement some kind of timer to send queued messages automatically.

In libbalsa/send.c I noticed that the sending_threads global variable is used in an unsafe way as it is accessed from multiple threads without always using a mutex.  The simple fix is to use atomic operations.  I'm not sure if the mutex send_messages_lock is actually needed, but it doesn't hurt IMO. BTW, I also ref the outbox and smtp_server objects before calling libbalsa_server_test_can_reach() which is somewhat extra-paranoid, but shouldn't be an issue, too.

In Peter's implementation, the function libbalsa_server_test_can_reach_full() will always report an unreachable server if the host contains a port number ("host:port") which is typical for SMTP.  I also fixed this issue.

Finally, I removed the debug flag from the sending stuff, as debugging in libnetclient is controlled by the G_MESSAGES_DEBUG environment variable.

Opinions?

Cheers,
Albrecht.

---
Patch details:
- libbalsa/send.c: use atomic operations for the sending state; check if the smtp server is reachable before sending; simplify some API's; replace the term "MTA" in user dialogues by "SMTP server" which is more common
- libbalsa/send.h: export simplified API
- libbalsa/server.c: ignore port in server host string
- src/balsa-message.c, src/balsa-mime-widget-message.c, balsa-mime-widget-vcalendar.c, src/main-window.c, src/sendmsg-window.c: use simplified API
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list

smtp-check-network.diff (16K) Download Attachment
attachment1 (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

JohnJackDoe
On 25 May 2017 18:07:15, Albrecht Dreß wrote:
snip
> Basically, when a specific smtp server is not reachable, a warning is  
> shown, and the messages are left in the outbox.  Unlike after an  
> error, the messages are /not/ tagged.  Therefore, simply choosing  
> "Send queued mail" will send the messages if possible, or just leave  
> them (untagged) in the outbox if the server is still unreachable.  I  
> did not implement some kind of timer to send queued messages  
> automatically.
snip

I would appreciate queued mail to be send automatically if the host is  
available again. So some kind of timer would be great.
--
Best regards

John Jack Doe
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Albrecht Dreß
Am 25.05.17 18:18 schrieb(en) [hidden email]:
> I would appreciate queued mail to be send automatically if the host is available again. So some kind of timer would be great.

I'll look into that after the review of the first part...

Thanks
Albrecht.
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list

attachment0 (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Peter Bloomfield
In reply to this post by Albrecht Dreß
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Albrecht:

On 05/25/2017 12:07:15 PM Thu, Albrecht Dreß wrote:

> Hi all!
>
> Am 23.05.17 00:58 schrieb(en) Peter Bloomfield:
>> The git "master" branch has some new code, for more careful checking of when a POP3 host can be reached. This should avert some warnings about failure to connect to POP3 hosts. Please report any issues with it!
>
> As a follow-up to this patch, I implemented the same feature for outgoing smtp connections, using Peter's new functions.
>
> Basically, when a specific smtp server is not reachable, a warning is shown, and the messages are left in the outbox.  Unlike after an error, the messages are /not/ tagged.  Therefore, simply choosing "Send queued mail" will send the messages if possible, or just leave them (untagged) in the outbox if the server is still unreachable.  I did not implement some kind of timer to send queued messages automatically.
>
> In libbalsa/send.c I noticed that the sending_threads global variable is used in an unsafe way as it is accessed from multiple threads without always using a mutex.  The simple fix is to use atomic operations.  I'm not sure if the mutex send_messages_lock is actually needed, but it doesn't hurt IMO. BTW, I also ref the outbox and smtp_server objects before calling libbalsa_server_test_can_reach() which is somewhat extra-paranoid, but shouldn't be an issue, too.
>
> In Peter's implementation, the function libbalsa_server_test_can_reach_full() will always report an unreachable server if the host contains a port number ("host:port") which is typical for SMTP.  I also fixed this issue.
>
> Finally, I removed the debug flag from the sending stuff, as debugging in libnetclient is controlled by the G_MESSAGES_DEBUG environment variable.
>
> Opinions?

Thanks for the patch, and the effort you put into it!

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, it's object-reffed in the LibBalsaServer code, so there's no need to object-ref it in lbs_process_queue.

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?

Peter
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlknkBMACgkQH1/UtbkqdPVn5wCbB5FmtLGngsKbXY7xXQ7VNWQf
4WsAn2waPVCBN/ugJ3c9Yk8R82tydFog
=8tem
-----END PGP SIGNATURE-----
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Albrecht Dreß
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!

> 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().

Thanks,
Albrecht.
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list

smtp-check-network_v2.diff (17K) Download Attachment
attachment1 (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Peter Bloomfield
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
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Peter Bloomfield
In reply to this post by Albrecht Dreß
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Albrecht:

On 05/26/2017 08:14:33 AM Fri, Albrecht Dreß wrote:
> Hi Peter:

> 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().

This is all working well!

…except for one occasional glitch:

(balsa:23011): Gtk-CRITICAL **: gtk_window_set_transient_for: assertion 'parent == NULL || GTK_IS_WINDOW (parent)' failed
Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.

The offending call is in ensure_send_progress_dialog, passing "parent" as the transient parent for the send-progress dialog, which is in turn called from lbs_process_queue_real, passing SendQueueInfo::parent. Gdb shows that send_info->parent is not balsa_app.main_window, which is supposed to be passed down from send_queued_mail_activated. It's also not been NULL, in the cases I've looked at.

The other two members of send_info do not seem to have been corrupted; outbox is still my outbox, and finder is still balsa_find_sentbox_by_url, So somehow just that third member is getting over-written, and I don't know how to detect that.

Thoughts, anyone?

Peter
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlkphtkACgkQH1/UtbkqdPXWewCfeOX4CWQWjiQpM5zP8J6JpLKP
Js8AoIsvJlFuvvqU4tpETjylVHdPO1kB
=N7Jn
-----END PGP SIGNATURE-----
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Peter Bloomfield
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/27/2017 10:02:01 AM Sat, Peter Bloomfield wrote:

> The offending call is in ensure_send_progress_dialog, passing "parent" as the transient parent for the send-progress dialog, which is in turn called from lbs_process_queue_real, passing SendQueueInfo::parent. Gdb shows that send_info->parent is not balsa_app.main_window, which is supposed to be passed down from send_queued_mail_activated. It's also not been NULL, in the cases I've looked at.

Oops--I was looking at the wrong code path: if it's an instant send, not a queue-and-send-queued, the parent is the compose window, and it sometimes gets destroyed during the async check for reachability. Adding a ref/unref should keep it alive long enough.

Sorry for the noise!

Peter
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlkpi7MACgkQH1/UtbkqdPXm2wCdGUxDm+m8yzLzVFwEmRpEIBjl
NAYAn08XG5gvgfbDNfBAr4j8Sqz89Who
=GJLw
-----END PGP SIGNATURE-----
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Albrecht Dreß
Hi Peter:

Am 27.05.17 16:22 schrieb(en) Peter Bloomfield:
>> The offending call is in ensure_send_progress_dialog, passing "parent" as the transient parent for the send-progress dialog, which is in turn called from lbs_process_queue_real, passing SendQueueInfo::parent. Gdb shows that send_info->parent is not balsa_app.main_window, which is supposed to be passed down from send_queued_mail_activated. It's also not been NULL, in the cases I've looked at.
>
> Oops--I was looking at the wrong code path: if it's an instant send, not a queue-and-send-queued, the parent is the compose window, and it sometimes gets destroyed during the async check for reachability. Adding a ref/unref should keep it alive long enough.

Hmmm, I wonder if this would really be the reasonable way...

I think the user expects that the message is either sent or queued /somehow/ whenever [s]he hits the send button.  Remember that the "real" send process may be delayed for some time, e.g. if a dial-up connection needs to be established (maybe ~30 seconds in this case).  Still showing the composer sounds impractical to me.

This means that the composer dialogue should be closed, regardless of the state of the send process.  Which in turn implies that the usual state would actually be the destroyed parent!

IMO, there are two proper ways to deal with this situation:
1) always use the main window as parent, when showing the send progress dialogue
2) use that status bar in the main window to show the send progress

However, #2 might conflict with a pop3 download progress...

What do you think?

Cheers,
Albrecht.
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list

attachment0 (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Testing network availability

Peter Bloomfield
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/27/2017 12:39:17 PM Sat, Albrecht Dreß wrote:

> Hi Peter:
>
> Am 27.05.17 16:22 schrieb(en) Peter Bloomfield:
>>> The offending call is in ensure_send_progress_dialog, passing "parent" as the transient parent for the send-progress dialog, which is in turn called from lbs_process_queue_real, passing SendQueueInfo::parent. Gdb shows that send_info->parent is not balsa_app.main_window, which is supposed to be passed down from send_queued_mail_activated. It's also not been NULL, in the cases I've looked at.
>>
>> Oops--I was looking at the wrong code path: if it's an instant send, not a queue-and-send-queued, the parent is the compose window, and it sometimes gets destroyed during the async check for reachability. Adding a ref/unref should keep it alive long enough.
>
> Hmmm, I wonder if this would really be the reasonable way...
>
> I think the user expects that the message is either sent or queued /somehow/ whenever [s]he hits the send button.  Remember that the "real" send process may be delayed for some time, e.g. if a dial-up connection needs to be established (maybe ~30 seconds in this case).  Still showing the composer sounds impractical to me.
>
> This means that the composer dialogue should be closed, regardless of the state of the send process.  Which in turn implies that the usual state would actually be the destroyed parent!

You're right, of course--not much point in using the async method if the compose window is held open until it finishes!

> IMO, there are two proper ways to deal with this situation:
> 1) always use the main window as parent, when showing the send progress dialogue
> 2) use that status bar in the main window to show the send progress
>
> However, #2 might conflict with a pop3 download progress...
>
> What do you think?

1) looks good to me. I was actually surprised to see the compose window code using the compose window as parent.

Best,

Peter
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlkpvmwACgkQH1/UtbkqdPXZZQCglr8K5Ife4e33qaE+avR57W8t
SdwAnA3DZ77lbKJLxqovqGv3bH1xJF2K
=ea+P
-----END PGP SIGNATURE-----
_______________________________________________
balsa-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/balsa-list
Loading...