Created attachment 248192 [details] [review]
Use g_strcmp0 instead of implement it as check_equal

Created attachment 248193 [details] [review]
allow to pass NULL for e_ews_connection_get_folder_sync

Created attachment 248194 [details] [review]
Bug #699349 - Listen for server change notifications

BUGS:
Beyonde those comented in the code, we have:
- Deleted contact reappears
- SEGVs in calendar/addressbook (I'm not sure that it us caused by this
patch but, at least, this patch can catalyze the process)

TODO:
- Use GSettings

Review of attachment 248192 [details] [review]:

It looks like a cleanup thing, not much related to the bug as such, but I've no problem with it, feel free to commit to master. Thanks.

Review of attachment 248193 [details] [review]:

Similar as the above, feel free to commit to master. Thanks.

Review of attachment 248194 [details] [review]:

The same concerns from book-backend apply to calendar-backend as well. Please do not forget of it (the code looks quite identical between the two). In any case, you wrote it in the way I would write it too, thus you've made a good job with it.

::: src/addressbook/e-book-backend-ews.c
@@ +3529,3 @@
+				ebews->priv->subscription_id,
+				NULL,
+				NULL);

you should free & NULL the ebews->priv->subscription_id after this call, thus it's not left there forever (and the re-enable would leak the memory).

@@ +3542,3 @@
+
+	/* Avoid block the UI */
+	g_thread_new (NULL, handle_notifications_cb, ebews);

a) there is no UI in the book backend, but I know what you mean, you mean the main thread
b) the returned GThread is leaking here, it needs g_thread_unref() on it
c) ref the ebews, thus it's not lost when the thread reaches its execution

@@ +3558,3 @@
+	sync_state = e_book_backend_sqlitedb_get_sync_data (priv->summary, priv->folder_id, NULL);
+	do
+	{

I  know you copy&pasted this code, but please move the '{' on the right place.

@@ +3565,3 @@
+		sync_state = NULL;
+
+		e_ews_connection_sync_folder_items_sync (

test returned values too. For example, if you pass-in wrong pointers, then the error will be NULL, but you'll still think the opration finished successfully. For the same reason it might be good to set includes_last_item to FALSE.

@@ +3595,3 @@
+			ebews_fetch_items (ebews, items_created, TRUE, NULL, priv->cancellable, &error);
+
+		if (error) {

please extend this. FOr example, if the error will be set right from items_deleted, then the part with items_created will break on already set error.

@@ +3642,3 @@
+			case E_EWS_EVENT_COPIED:
+				if (g_strcmp0 (event->folder_id, ebews->priv->folder_id) == 0 ||
+						g_strcmp0 (event->old_folder_id, ebews->priv->folder_id) == 0)

please align it with the above g_strcmp0()

@@ +3651,3 @@
+
+	if (update_folder)
+		g_thread_new (NULL, ews_update_items_thread, ebews);

similar notes as at the above g_thread_new() call.

@@ +3670,3 @@
+	 * FIXME:
+	 * - Why is it called successfully more than once?
+	 */

hmm, good question

::: src/camel/camel-ews-store.c
@@ +657,3 @@
+{
+	struct ScheduleUpdateData *sud = user_data;
+	CamelEwsStore *ews_store = g_object_ref (sud->ews_store);

too late for reffing here

@@ +737,3 @@
+
+	sud = g_new0 (struct ScheduleUpdateData, 1);
+	sud->ews_store = ews_store;

better to ref the store here, and unref in free_schedule_update_data()

@@ +743,3 @@
+		NULL,
+		folder_list ? camel_ews_folder_list_update_thread : camel_ews_folder_update_thread,
+		sud);

leaking returned GThread

@@ +873,3 @@
+	gboolean update_folder_list = FALSE;
+	gboolean update_folder = FALSE;
+	GHashTable *folder_ids = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);

allocate only after runtime checks, otherwise you can leak the memory

@@ +928,3 @@
+	 * FIXME:
+	 * - Why is it only notified once?
+	 * - Should I do it in a new thread? (It could, at least, avoid to block the UI)

The change notifications are usually received in the main thread, thus yes, please create a new thread and enable notifications from there (as you do in book/cal backends)

@@ +1073,3 @@
+
+		g_signal_handlers_disconnect_by_func (
+				CAMEL_EWS_SETTINGS (settings),

no need to type-cast here

::: src/camel/camel-ews-utils.c
@@ +114,3 @@
+			 * FIXME:
+			 * We are never TRUE here. As result, the UI is not updated after a folder be deleted
+			 * Why? Should we subscribe every new-added folder?

Hmm, once the CamelStore advertises that it uses folder subscriptions, then the folder tree in evolution listens mainly for folder subscribed/unsubscribed events, and ignores folder created/deleted events, basically (you should send both events in correct order). I would not set the CAMEL_FOLDER_SUBSCRIBED flag on each folder in CamelFolderInfo, rather make this unconditional, and use the flag when necessary (aka do not change other things, only this line).

::: src/server/camel-ews-settings.c
@@ +402,3 @@
+			"listen-notifications",
+			"Listen Notifications",
+			"Wheter to listen for server notification",

typo, 'Whether'

::: src/server/e-ews-connection.c
@@ +100,3 @@
 	EEwsServerVersion version;
+
+	GHashTable *known_notifications;

As you access this member from multiple threads, you should also have a GMutex to guard the variable(s).

@@ +228,3 @@
+	/*
+	 * FIXME:
+	 * Why does evo get stucked here?

I would bet for thread safety, the GHashTable is not thread safe on its own.

@@ +253,3 @@
+			GSList *events = NULL;
+
+			e_ews_connection_get_events_sync (

what if the call will fail?

@@ +270,3 @@
+		end_time = g_get_monotonic_time () + (G_TIME_SPAN_SECOND * cnc->priv->notification_poll_seconds);
+
+		e_flag_wait_until (cnc->priv->notification_flag, end_time);

should not you also e_flag_clear() here, to make sure you'll not be fired immediately, once one of the e_flag_set() is called? Though, ideally, you should use here a GCond directly, which will share the mutex to guard the variable(s).

@@ +1635,3 @@
+			G_TYPE_NONE,
+			1,
+			G_TYPE_POINTER);

I would also use a g_cclosure_marshal_VOID__POINTER in the definition, as it is at https://git.gnome.org/browse/evolution/tree/shell/e-shell.c#n968 Also see the indentation used there.

@@ +1697,3 @@
+		g_settings_set_int (settings, "notification-poll-seconds", notification_poll_seconds);
+	}
+#endif

As we spoke on IRC, please make this part of CamelEwsSettings, thus the interval will be changeable per-account. Also add an option into the mail account properties, probably below the Global Address List options, or near the checkbox for the Listen for server notifications, and the option will be disabled, if the listening will be disabled. The actual poll timeout value will be shared by all backends.

@@ +8823,3 @@
+	GSimpleAsyncResult *simple;
+
+	g_return_if_fail (cnc != NULL);

do not trust passed-in parameters, add there checks for the 'subscription_id' as well (and make sure the code will not call this function with it NULL.)

@@ +8966,3 @@
+	for (param = e_soap_parameter_get_next_child (param); param != NULL; param = e_soap_parameter_get_next_child (param)) {
+		for (event_type = 0; events[event_type] != NULL; event_type++) {
+			if (g_strcmp0 ((const gchar *)param->name, events[event_type]) != 0)

coding style: space before 'param'

@@ +9142,3 @@
+ * Deleted and Moved).
+ * Pair function for this is e_ews_connection_disable_notifications().
+ * The notification os received to the caller with the "server-notification" signal.

s/os/is/

@@ +9170,3 @@
+
+		for (i = 0; default_events_names[i] != NULL; i++)
+			default_events = g_slist_prepend (default_events, g_strdup (default_events_names[i]));

doe sit make sense to g_strdup() the pointers here? Also, what about defining known/default events in a global static variable, rather than define it twice, once here and once at ews_handle_events_param()? Though I see there is the extra 'StatusEvent'.

::: src/server/e-ews-connection.h
@@ +220,3 @@
+	gchar *folder_id;
+	gchar *old_folder_id;
+} EEwsEvent;

There are different kinds of events, thus what about not using EEwsEvent(Type), but EEwsNotificationEvent(Type) instead? Just to make it clear what 'event' kind this is about.

Created attachment 248716 [details] [review]
Bug #699349 - Listen for server change notifications

Review of attachment 248716 [details] [review]:

Please note that some comments apply across multiple files, because of the similarity in book and cal backends, and sometimes in camel as well. Also check usage of PRIV_LOCK/UNLOCK in backends, maybe you might want to use it on some places as well.

::: src/addressbook/e-book-backend-ews.c
@@ +2957,3 @@
 		GSList *members = NULL;
 		gboolean includes_last;
+		gboolean ret;

why do you redefine the 'ret' here? It's defined at the top of the function.

@@ +3516,3 @@
+		folders = g_slist_prepend (folders, ebews->priv->folder_id);
+		e_ews_connection_enable_notifications (
+				ebews->priv->cnc,

What if the priv->cnc got NULL meanwhile? The function might also be called 'handle_notifications_thread' rather than '..._cb', to distinguish between real callback and a thread function.

@@ +3527,3 @@
+		e_ews_connection_disable_notifications (
+				ebews->priv->cnc,
+				ebews->priv->subscription_id,

apart of test of validity on priv->cnc, also make sure the ebews->priv->subscription_id is not NULL here (and is NULL when actually listening the notifications).

@@ +3550,3 @@
+	ebews->priv->listen_notifications = camel_ews_settings_get_listen_notifications (ews_settings);
+	/* Let's use the same timeout used to connection, but in minutes */
+	ebews->priv->notifications_timeout = camel_ews_settings_get_timeout (ews_settings)/60;

coding style: missing spaces around /60.

@@ +3714,3 @@
+		GThread *thread;
+
+		thread = g_thread_new (NULL, ews_update_items_thread, ebews);

ref the ebews here?

@@ +3764,3 @@
+
+			/* Let's use the same timeout used to connection, but in minutes */
+			priv->notifications_timeout = camel_ews_settings_get_timeout (ews_settings)/60;

coding style: missing spaces around /60

@@ +3773,3 @@
+					NULL,
+					priv->notifications_timeout,
+					&priv->subscription_id,

if I did not overlook anything, then the priv->subscription_id can be leaked if you were disconnected and then reconnected due to connection change or anything like that.

@@ +3909,3 @@
+					NULL);
+
+			g_free (priv->subscription_id);

do this free always, regardless priv->cnc and priv->listen_notifications values.

::: src/calendar/e-cal-backend-ews.c
@@ +607,3 @@
 
+static gpointer
+handle_notifications_cb (gpointer data)

similar notes as in the addressbook file, here name it with suffix "_thread", and coding style with missing  spaces around /60

@@ +754,1 @@
+	if (ret) {

you do this always, even in the offline mode, where priv->cnc is most likely NULL.

@@ +3782,3 @@
 {
 	ECalBackendEwsPrivate *priv;
+	gboolean ret;

the variable seems to be useless here.

@@ +3966,3 @@
+		g_slist_free_full (items_created, g_object_unref);
+		g_slist_free_full (items_updated, g_object_unref);
+		g_slist_free_full (items_deleted, g_free);

expect double-free in case of cal_backend_ews_process_folder_items() returning FALSE

@@ +3974,2 @@
 	g_free (new_sync_state);
 	g_free (old_sync_state);

Do not forget of g_clear_error (&error); here

::: src/camel/camel-ews-provider.c
@@ +54,3 @@
+	   * user can select how long the time between poll the
+	   * notifications should be. */
+	  N_("Check for new _messages every %s minutes"), "0:1:0:60" },

Rather "Check for changes every %s minutes", because it's not only about new messages.

::: src/camel/camel-ews-store.c
@@ +698,3 @@
+				created, /* freed in the function */
+				deleted, /* freed in the function */
+				updated, /* freed in the function */

then add an else to the 'if' and free the memory there.

@@ +724,3 @@
+			continue;
+
+		camel_folder_refresh_info_sync (folder, sud->cancellable, NULL);

stop on failure here?

@@ +1008,3 @@
+			hnd->folders = camel_ews_store_summary_get_folders (ews_store->summary, NULL);
+	} else {
+		gchar *inbox = camel_ews_store_summary_get_folder_id_from_name (ews_store->summary, "Inbox");

Rather use the folder type, than a name, here. Note it can sometimes return NULL (usually on error).

@@ +1014,3 @@
+	ews_store->priv->listen_notifications = camel_ews_settings_get_listen_notifications (ews_settings);
+	/* Let's use the same timeout used to connection, but in minutes */
+	ews_store->priv->notifications_timeout = camel_ews_settings_get_timeout (ews_settings)/60;

coding style

@@ +1017,3 @@
+	if (ews_store->priv->notifications_timeout < 1)
+		ews_store->priv->notifications_timeout = 1;
+	thread = g_thread_new (NULL, handle_notifications_cb, hnd);

"_thread" not "_cb" suffix

@@ +1032,2 @@
 	CamelSession *session;
+

why the new empty line? I suppose it's a leftover or typo.

@@ +1083,3 @@
+			} else {
+				gchar *inbox;
+				inbox = camel_ews_store_summary_get_folder_id_from_name (ews_store->summary, "Inbox");

try with type, not name (see above)

@@ +3356,2 @@
 	/* Chain up to parent's finalize() method. */
 	G_OBJECT_CLASS (camel_ews_store_parent_class)->finalize (object);

hmm, nothing to add to 'dispose'?

::: src/server/e-ews-connection.c
@@ +294,3 @@
+
+			if (events != NULL)
+				g_signal_emit (cnc, signals[SERVER_NOTIFICATION], 0, events);

can the signal handler call anything what would require the notification lock? If so, then define the lock as recusrsive

::: src/server/e-ews-connection.h
@@ +223,3 @@
+
+EEwsNotificationEvent
+		*e_ews_notification_event_new	(void);

move the '*' one line up, beside the type name

*** Bug 701440 has been marked as a duplicate of this bug. ***

Milan,

I'm still with a few problems (please, take a look in the commit message and in the "XXX" in the code's comment).
I hope this version can be really close to the end version.

Created attachment 255540 [details] [review]
Bug #699349 - Listen for server change notifications

Why ews_disconnect_sync() is called every time we close
(clicking in the "Ok" button) the Account Editor?

(In reply to comment #11)
> Why ews_disconnect_sync() is called every time we close
> (clicking in the "Ok" button) the Account Editor?

It's because the changes you did in Account Properties could influence also connection settings, thus evolution tries to disconnect the service, clean-up folder tree from the old data and then reconnect again, to obtain correct folder list and so on.

(In reply to comment #12)
> (In reply to comment #11)
> > Why ews_disconnect_sync() is called every time we close
> > (clicking in the "Ok" button) the Account Editor?
> 
> It's because the changes you did in Account Properties could influence also
> connection settings, thus evolution tries to disconnect the service, clean-up
> folder tree from the old data and then reconnect again, to obtain correct
> folder list and so on.

It's a problem :-(
We are listening for changes in listen-notification and check-all properties. Once it occurs, we enable the notifications on the server and then ews_disconnect_sync() disables it.

I was expecting that ews_connect_sync() could be called again and then everything would be fine, but it is not what is happening. :-(

The easiest way to workaround it is only support the changes after restart Evo.
Do you have another idea?

Review of attachment 255540 [details] [review]:

I did not run the code yet. Some of the notes apply in general, because of the similarity of the code in all three parts (book/cal/camel), thus please make sure you apply respective changes in all of them.

::: src/addressbook/e-book-backend-ews.c
@@ +3592,3 @@
+ews_update_items_thread (gpointer data)
+{
+	EBookBackendEws *ebews = data;

I would pass in reffed ebews, just to make sure it'll not disappear before the thread is executed.

@@ +3605,3 @@
+		GSList *items_updated = NULL;
+		GSList *items_deleted = NULL;
+		GError *error = NULL;

redefinition of a variable ('error' is defined in the parent's scope already)

@@ +3792,3 @@
+		priv->listen_notifications = camel_ews_settings_get_listen_notifications (ews_settings);
+
+		if (ebews->priv->listen_notifications) {

cannot you call ebews_listen_notifications_cb() here? You know, to save a little bit of code duplication.

::: src/calendar/e-cal-backend-ews.c
@@ +629,3 @@
+		e_ews_connection_disable_notifications (
+				cbews->priv->cnc,
+				cbews->priv->subscription_id,

you should check priv->subscription_id before calling the function, should you not?

@@ +4086,3 @@
+							priv->cancellable,
+							&error)) {
+					if (g_error_matches (

there was '!g_error_matches', same as this was done always, not only when the stored sync data was incorrect.

@@ +4111,3 @@
+		g_slist_free_full (items_created, g_object_unref);
+		g_slist_free_full (items_updated, g_object_unref);
+		g_slist_free_full (items_deleted, g_free);

set all the three variables to NULL

::: src/camel/camel-ews-store.c
@@ +651,3 @@
+
+	if (sud->cancellable != NULL)
+		g_object_unref (sud->cancellable);

g_clear_object is a pretty convenient function, it reduces the two lines into one: g_clear_object (&sud->cancellable);
and you get the sud->cancellable nullified as well/for free.

@@ +703,3 @@
+				created, /* freed in the function */
+				deleted, /* freed in the function */
+				updated, /* freed in the function */

that means that none of the four is freed, if the code doesn't reach this place. I do not like this 'freed in the function' API.

@@ +716,3 @@
+{
+	struct ScheduleUpdateData *sud = user_data;
+	CamelEwsStore *ews_store = g_object_ref (sud->ews_store);

this ref is not needed, because it's referenced within the 'sud' structure already

@@ +726,3 @@
+		GError *error = NULL;
+
+		folder = camel_store_get_folder_sync (CAMEL_STORE (ews_store), folder_name, 0, sud->cancellable, NULL);

this returns reffed GObject

@@ +739,3 @@
+
+	g_slist_free_full (ews_store->priv->update_folder_names, g_free);
+	ews_store->priv->update_folder_names = NULL;

it would be good to have some locking around the variable, to avoid race conditions when it is accessed from multiple threads; it's the UPDATE_LOCK, right?

@@ +832,3 @@
+	ews_store->priv->update_folder_id = g_timeout_add_seconds_full (
+								G_PRIORITY_LOW,
+								5,

hmm, hard-coded 5 seconds? what about the option in account properties? ditto in schedule_folder_list_update()

@@ +908,3 @@
+	gboolean update_folder_list = FALSE;
+	gboolean update_folder = FALSE;
+	GHashTable *folder_ids = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);

if one of the below runtime checks succeeds, then you leak this pointer.

@@ +943,3 @@
+				break;
+			default:
+				return;

break please (memory leak introduced, otherwise)

@@ +961,3 @@
+
+static void
+handle_notifications_data_free (struct HandleNotificationsData *hnd) {

coding style issue

@@ +1027,3 @@
+	/*
+	 * XXX:
+	 * Why this code is never reached?

hard to tell off head, though my wild guess would be that the "OK" in account properties disconnects the store, technically frees it and runs a new store with these updated settings, thus the value didn't change 'in the previous instance of the CamelEwsStore/CamelEwsSettings'.

@@ +1085,3 @@
+	 * XXX:
+	 * Why this function is being called even when I just
+	 * change the check-all property in the Account Editor?

ditto with the above XXX comment?

@@ +1190,3 @@
 
+	g_signal_connect_swapped (
+			priv->connection,

connection can be NULL, if it failed

@@ +1196,3 @@
+
+	g_signal_connect_swapped (
+			ews_settings,

a) ews_settings can be used uninitialized, if it failed above;
b) be careful here, ews_settings is there for whole lifetime of the CamelEwsStore, while 'connection' can be added and removed on demand (connection state changes).

::: src/server/e-ews-connection.c
@@ +238,3 @@
+
+	NOTIFICATION_LOCK (cnc);
+	g_hash_table_foreach (cnc->priv->known_notifications, call_stop_notifications, cnc);

I see a possible deadlock here. You hold the notification lock, then call call_stop_notifications(), which calls stop_notification(), which tries to get the notification lock. And here we go.

@@ +262,3 @@
+		NOTIFICATION_LOCK (cnc);
+
+		if (g_cond_wait_until (&cnc->priv->notification_cond, &cnc->priv->notification_lock, end_time)) {

starting with end_time = 0 looks odd to me, or at least pass it to g_cond_wait_until()

@@ +288,3 @@
+					g_clear_error (&error);
+				}
+				continue;

if reached, and there are known_notifications, then you get a deadlock

@@ +294,3 @@
+
+			if (events != NULL)
+				g_signal_emit (cnc, signals[SERVER_NOTIFICATION], 0, events);

who does free the 'events'?

@@ +364,3 @@
+ * XXX:
+ * Keeping this function for debug purposes.
+ * Should I remove it?

you can keep it. You introduced e-ews-debug.h/.c in some patch, then this can be moved there, only fix the coding-style inside the function.

@@ +8880,3 @@
+	/*
+	 * EWS server version earlier than 2010 doesn't have support to "SubscribeToAllFolders" attribute.
+	 * So, if we want to do this, we need to do explicity, passing the a list with all folders, otherwise

typos: 'explicity', 'the a'

@@ +8898,3 @@
+			NULL,
+			cnc->priv->version,
+			E_EWS_EXCHANGE_2007_SP1,

As long as "SubscribeToAllFolders" was introduced in 2010, you should use that version request here as well, right?

@@ +8958,3 @@
+
+	if (async_data->subscription_id == NULL || async_data->watermark == NULL)
+		return FALSE;

one of the above can leak here, theoretically.

@@ +9108,3 @@
+
+static EEwsNotificationEvent *
+get_folder_event_info (ESoapParameter *param, EEwsNotificationEventType event_type)

coding style (and the other two below functions as well)

@@ +9175,3 @@
+			if (g_strcmp0 ((const gchar *) param->name, default_events_names[event_type]) != 0)
+				continue;
+			break;

heh, this construction is not that common, is it? More straightforward '... == 0) break;' seems also easier to understand.

@@ +9393,3 @@
+		goto exit;
+
+	g_hash_table_replace (cnc->priv->known_notifications, sub, watermark);

here should be used the NOTIFICATION_LOCK as well.

::: src/server/e-ews-connection.h
@@ +1116,3 @@
+						 GCancellable *cancellable,
+						 GError **error);
+gboolean	e_ews_connection_disable_notifications

please name the last two (enable/disable_notifications) with a "_sync" suffix.

(In reply to comment #13)
> We are listening for changes in listen-notification and check-all properties.
> Once it occurs, we enable the notifications on the server and then
> ews_disconnect_sync() disables it.
> 
> I was expecting that ews_connect_sync() could be called again and then
> everything would be fine, but it is not what is happening. :-(
> 
> The easiest way to workaround it is only support the changes after restart Evo.
> Do you have another idea?

Let's keep it as is for now. It sounds like a bug in evolution, on the first look.

Created attachment 255617 [details] [review]
Bug #699349 - Listen for server change notifications

I compiled the patch and run evolution. I enblaed "listen for notifications" and pressed OK button.I see this warning twice on the console:
> (evolution:19801): libeews-CRITICAL **:
> e_ews_connection_disable_notifications_sync: assertion
> `subscription_id != NULL' failed

I setup a refresh interval for 1 minute, then I sent myself a message from a different account. In time of the refresh interval, where I would expect it, is shown on console twice:

> (evolution:19801): GLib-GObject-CRITICAL **: g_object_ref: assertion
> `G_IS_OBJECT (object)' failed

and after another minute (or so) once this:

> (evolution:19801): camel-ews-provider-CRITICAL **: run_update_thread:
> assertion `cancellable != NULL' failed

the outcome is that the Inbox is not updated, UI still shows the same unread count as before.

(In reply to comment #17)
> I compiled the patch and run evolution. I enblaed "listen for notifications"
> and pressed OK button.I see this warning twice on the console:
> > (evolution:19801): libeews-CRITICAL **:
> > e_ews_connection_disable_notifications_sync: assertion
> > `subscription_id != NULL' failed

My guess is that it's occurring due that problem we have discusses about "Ok" button triggering the ews_disconnect_sync().
It's one of the reasons I believe we should not apply the changes immediately.

> 
> I setup a refresh interval for 1 minute, then I sent myself a message from a
> different account. In time of the refresh interval, where I would expect it, is
> shown on console twice:
> 
> > (evolution:19801): GLib-GObject-CRITICAL **: g_object_ref: assertion
> > `G_IS_OBJECT (object)' failed
> 
> and after another minute (or so) once this:
> 
> > (evolution:19801): camel-ews-provider-CRITICAL **: run_update_thread:
> > assertion `cancellable != NULL' failed
> 
> the outcome is that the Inbox is not updated, UI still shows the same unread
> count as before.

Argh, those ones are new, sorry.
I had it completely tested before the last rebase (after we branch 3.10) and probably I messed up something in this process. Let me check and update the patch.

Created attachment 255693 [details] [review]
Bug #699349 - Listen for server change notifications

Created attachment 255694 [details] [review]
Bug #699349 - Listen for server change notifications

Comment on attachment 255694 [details] [review]
Bug #699349 - Listen for server change notifications

Just for a record, from our yesterday IRC chat, we decided to not use pull, neither push notification, but streaming notifications. Those are available since Exchange server 2010 SP1, and allows almost-realtime notifications, without periodical polling. That is what we want. The good thing is that the most of the code will remain the same, only some internal things in EEWsConnection will need changes.

Milan,

I've been some troubles to test the Kerberos authentication (we can discuss it on IRC, don't worry), but this version is really close to the "final" version.
Could you take a look and start the review process? I don't think we will have drastic changes from now to the next versions (hope we don't need a lot of next versions)

Created attachment 259510 [details] [review]
Bug #699349 - Listen for server change notifications

Review of attachment 259510 [details] [review]:

From the functionality point of view: it works nicely, good job. Now the worse part:

Compilation warnings:
In file included from /usr/include/glib-2.0/gobject/gobject.h:30:0,
                 from /usr/include/glib-2.0/gobject/gbinding.h:31,
                 from /usr/include/glib-2.0/glib-object.h:25,
                 from /build/local/include/evolution-data-server/camel/camel-address.h:29,
                 from /build/local/include/evolution-data-server/camel/camel.h:29,
                 from e-cal-backend-ews.c:34:
e-cal-backend-ews.c: In function 'e_cal_backend_ews_open':
/usr/include/glib-2.0/gobject/gsignal.h:506:27: warning: 'ews_settings' may be used uninitialized in this function [-Wmaybe-uninitialized]
     g_signal_connect_data ((instance), (detailed_signal), (c_handler), (data), NULL, G_CONNECT_SWAPPED)
                           ^
e-cal-backend-ews.c:709:20: note: 'ews_settings' was declared here
  CamelEwsSettings *ews_settings;
                    ^
In file included from /usr/include/glib-2.0/gobject/gobject.h:30:0,
                 from /usr/include/glib-2.0/gobject/gbinding.h:31,
                 from /usr/include/glib-2.0/glib-object.h:25,
                 from /usr/include/glib-2.0/gio/gioenums.h:30,
                 from /usr/include/glib-2.0/gio/giotypes.h:30,
                 from /usr/include/glib-2.0/gio/gio.h:28,
                 from /build/local/include/evolution-data-server/libedataserver/e-cancellable-locks.h:28,
                 from /build/local/include/evolution-data-server/libedataserver/libedataserver.h:24,
                 from /build/local/include/evolution-data-server/libebook-contacts/libebook-contacts.h:24,
                 from /build/local/include/evolution-data-server/libedata-book/libedata-book.h:24,
                 from e-book-backend-ews.c:41:
e-book-backend-ews.c: In function 'e_book_backend_ews_open':
/usr/include/glib-2.0/gobject/gsignal.h:506:27: warning: 'ews_settings' may be used uninitialized in this function [-Wmaybe-uninitialized]
     g_signal_connect_data ((instance), (detailed_signal), (c_handler), (data), NULL, G_CONNECT_SWAPPED)
                           ^
e-book-backend-ews.c:3782:20: note: 'ews_settings' was declared here
  CamelEwsSettings *ews_settings;
                    ^

::: src/addressbook/e-book-backend-ews.c
@@ +2875,3 @@
 	contact = ebews_get_dl_info (ebews, id, d_name, members, error);
 	if (contact == NULL)
+		return TRUE;

shouldn't you return false here, indicating a failure? (couple more times below too)

@@ +3557,3 @@
+	EBookBackendEws *ebews = data;
+
+	if (ebews->priv->cnc == NULL)

You should PRIV_LOCK/UNLOCK here (in whole function) as well; similar with calendar, to avoid multithreading issues. [It makes me wonder if we would be able, one day, extract some common parts into a base EWS backend structure and reuse this duplicated code from it. It's far far far out of scope of this bug report, of course.]

@@ +3655,3 @@
+				ebews_forget_all_contacts (ebews);
+
+				if (!e_ews_connection_sync_folder_items_sync (

you can call this function with non-NULL 'error', if the previous call e_book_backend_sqlitedb_set_sync_data() sets it.

::: src/calendar/e-cal-backend-ews.c
@@ +3953,3 @@
+		if (comp) {
+			if (!ews_cal_delete_comp (cbews, comp, item_id))
+				goto exit;

it returns TRUE, even if the function failed

@@ -3895,3 @@
 	old_sync_state = g_strdup (e_cal_backend_store_get_key_value (priv->store, SYNC_KEY));
 	do {
-		includes_last_item = TRUE;

keep there this line (and add it into the book backend code too)

@@ +4111,3 @@
 	ews_refreshing_dec (cbews);
 
+	if (!ret) {

you can safely do this always, not only if !ret, no?

::: src/camel/camel-ews-provider.c
@@ +50,3 @@
+	{ CAMEL_PROVIDER_CONF_CHECKBOX, "listen-notifications", NULL,
+	  N_("_Listen for server change notifications"), "1" },
+	{ CAMEL_PROVIDER_CONF_CHECKSPIN, "poll-minutes", "listen-notifications",

This sub-option is obsolete now, isn't it? At least it's not visible in UI, and might not be used anywhere in the code as well.

::: src/camel/camel-ews-utils.c
@@ -117,3 @@
-			camel_ews_store_summary_remove_folder (ews_summary, fid, &error);
-
-			if ((fi->flags & CAMEL_FOLDER_SUBSCRIBED) != 0) {

you cannot remove this test, subscriptions are used for public folders only.

::: src/server/e-ews-connection.c
@@ +215,3 @@
 
+void
+e_ews_notification_event_free (EEwsNotificationEvent *event)

be NULL-friendly here

@@ +9460,3 @@
+
+/* Enables server notification on a folder, or on a whole store, if folders is NULL.
+ * the events can be NULL to obtain the default notifications (Copied, Created,

Capital 'T' at the beginning of the line.

@@ +9481,3 @@
+
+	g_return_val_if_fail (cnc != NULL, FALSE);
+	g_return_val_if_fail (*subscription_id == NULL, FALSE);

check also for 'subscription_id != NULL', to have the checking complete

::: src/server/e-ews-notification.c
@@ +62,3 @@
+               const gchar *expected_name)
+{
+	/* Do not call this directory, use CHECK_ELEMENT macro instead. */

s/directory/directly/

@@ +147,3 @@
+	g_return_if_fail (notification->priv->connection == NULL);
+
+	notification->priv->connection = connection;

OK, so you do not ref the connection, which makes sense, to avoid cross-references, but what it will do if the connection will be freed before the notification is freed? It can happen when another thread is in the middle of processing event signals (g_signal_emit_... references the object first, during (almost) whole processing of this function).

@@ +420,3 @@
+	 * 2. Search for </Envelope> in notification->priv->chunk->data
+	 * 3.1 </Envelope> is not found: Do nothing. Waiting for the next chunk
+	 * 3.2 </Envelope> is found: Get the pair <Envelope>...</Envelope> and handle it

You mention <Envelope> here, but there is no checking for its existence. The server should return correct data, thus I've no problem with it (as long as the other code will not panic, if this occurs), I'm only mentioning it here.

@@ +458,3 @@
+		chunk_len = notification->priv->chunk->len;
+
+		if (chunk_len == 0) {

it's not obvious that 'chunk_len == 0' implies 'chunk_str = NULL', thus I'd do this differently, with some more obvious test in the 'while' conditions (like testing for chunk_len too).

::: src/server/e-soap-message.h
@@ +53,3 @@
 						 const gchar *env_prefix,
+						 const gchar *env_uri,
+						 gboolean standard_handle);

what about calling it "standard_handlers"? 'handle' and 'handlers' is slightly different.

Created attachment 259592 [details] [review]
Bug #699349 - Listen for server change notifications

Review of attachment 259592 [details] [review]:

functional issue:
I have setup "check in all folders" in mail Receiving Options, and changes from Inbox are properly propagated to UI, but if I change something in a subfolder, then the change doesn't get propagated into UI, even I see the event was received.

I didn't read the patch at all otherwise, maybe except of (on IRC mentioned) the connection timeout being currently set to 10 (it can be up to 30, but to be honest, 10 works for me too).

Created attachment 259627 [details] [review]
Bug #699349 - Listen for server change notifications

Review of attachment 259627 [details] [review]:

Just fix the last thing and commit to master. Thanks.

::: src/server/e-ews-notification.c
@@ +188,3 @@
+
+	if (priv->connection) {
+		g_object_weak_ref (

unref, not ref

Created attachment 259647 [details] [review]
Bug #699349 - Listen for server change notifications

Attachment 259647 [details] pushed as 16ed93c (evolution-ews 3.11.2+)

*** Bug 712135 has been marked as a duplicate of this bug. ***

Is this merged to master? 
I checked out and built the master (3.11.3) using jhbuild, however, I don't see notifications appearing.

For example, when a new message arrives, it appears immediately on my Activesync mobile and IMAP thunderbird, but not on Evolution EWS. If I do a manual sync, then it's OK.

Likewise, if I mark a message as unread in Thunderbird or mobile, the change is not immediately visible on Evo.

Could I be missing some configuration option?

(In reply to comment #32)
> Is this merged to master? 

Yes, it was pushed as 16ed93c (evolution-ews 3.11.2+)

> I checked out and built the master (3.11.3) using jhbuild, however, I don't see
> notifications appearing.

It should work.

> 
> For example, when a new message arrives, it appears immediately on my
> Activesync mobile and IMAP thunderbird, but not on Evolution EWS. If I do a
> manual sync, then it's OK.
> 
> Likewise, if I mark a message as unread in Thunderbird or mobile, the change is
> not immediately visible on Evo.

This is exactly what we expect from this commit.

> 
> Could I be missing some configuration option?

Do you have the "Listen for server change notification" checkbox (Account Preferences -> Receiving Options, under Check for New Email") checked?

I'm not responsive until 11th December, sorry, but after that could you ping me in the #evolution channel (at irc.gnome.org) and then I help you properly, if it's still not working for you?

Best Regards,

Hi I tested it again today upon your mail. It does work this time. So the bug can remain closed. I do not know why it did not work before, but if it happens again, I will re-post here.

Hi Fabiano,
I'm trying this again, but I can confirm that it's not working. My evolution-ews is 3.11.90, plus few handpicked patches from newer commits but nothing related to the "listen for changes".

My evo itself is the webkit-composer branch compiled from master, latest rebase with current Evolution on Feb 6th, 2014. I don't know if your patch needs some newer patch in Evo.

"listen for server change notifications" is selected in the options. When a new  mail arrives, my phone picks it up instantly, Evo never picks it until the next period sync or manual sync. 

I'll try to catch you in irc but I'm writing here anyway, in case you could give any hint on how to debug this.

Can you run Evo using EWS_DEBUG=2 and send us the output?
That would be really helpful.

Ah, please, be careful about exposing personal info with the EWS_DEBUG. :-)

Thanks, here it is. I can reproduce it. I sent 3 emails and none of them popped up in my mailbox for 20 minutes, then the periodic sync kicked in an synced them.

In the logs, I can see some 401 Unauthorized messages.
I can also see logs of Exchange 2007 SP1 and EXchange 2010 SP2 servers. Since this is a large company, although I know the hostname of my server, I can't guarrantee which server I will connect in case there's mixed 2007 and 2010 Exchange servers (in case this is related) in the server farm.

Created attachment 273708 [details]
EWS Server Change notification issue

Emre, this feature only works with Exchange 2010 SP1+.
I could see in your log a few "GetStreamingEvents", which is what we use to get the events and update.

Hmmm. Is there any way to force (in your server farm) to always connect to a "newer" server?

Fabiano, is there something in the logs that tells you that I'm connecting to an older server? I see both Exchange 2010 SP2 and 2007 SP1, so how do you know that the problem is on this?

I can see in the logs that some server is reporting its version number major 14 etc which is Exchange 2010 SP2.
When I connected from Outlook in a VM, it reported same version number which is 2010 SP2.

What I can do is to try my chances in getting the IP of that server, if I'm lucky again to hit it when DNS is resolved to it, or try each IP one by one until it works.

Please let me know if the root cause of the problem is really this issue.

I'm not sure if this is the problem, just mentioned that *if* you're trying to use it an older server, it's not expected to work at all.
From your log, what I see here is:
- The subscription is done properly
- GetStreamingEvents happens, what means that the server poke you about one change
- Timeout occurred

Once the timeout occurs, the expected behavior is that we subscribe to the server again and it should be transparent for you.

Do you mind helping a bit more to debug this?

Please, try:
- Run all evolution and factories using EWS_DEBUG=2 and keep the log
(the factories are located on $PREFIX/libexec/ and are evolution-addressbook-factory and evolution-calendar-factory, you want to pass -w to them)
- Run Evolution with EWS_DEBUG=2 and keep the log
- Stress your email/calendar/address book a bit, by sending/copying/moving emails/appointments/contacts
- Send me the log

I'm always online on IRC, #evolution channel, nickname: fidencio.
Try to catch me between 07:00AM~01:00AM in CEST timezone, but please, try to do that by Wednesday, 'cause I'll be travelling for a few days and only will be back on 22 (so, sorry if I'll be unresponsive for a while).

Fabiano, I've started evolution and the factories according to your advice today and collected logs. However, I recognized that it's now working properly. This looks like I'm in a mixed 2007 & 2010 Exchange server environment. 

I've obtained the IP address of the server using netstat and hardcoded it in /etc/hosts. I will monitor with this and report back if i face the issue agai, together with the logs.

You may consider adding a notification to the user that server change notifications won't work if the ews backend sees an old server.

Hi,
I'm again not able to get notifications for new mails, ie Evo does not refresh showing me new mails that arrive. Server is Exchange 2010 SP1, connected with EWS.

I got all debug logs, but I can't post here due to sensitive content. I'll try to catch you on IRC as I can't find your email address.

Get a backtrace of running evolution in the state of no notifications being received, with debuginfo available from evolution-ews at least.
   $ gdb --batch --ex "t a a bt" -pid=`pidof evolution` &>bt.txt
Please check the bt.txt for any private information, like passwords, email address, server addresses,... I usually search for "pass" at least (quotes for clarity only).

To have the notifications working you might have there a thread with ews notifications in the name.

Hi Milan, this command works when I run it as root, but exits in a 1 or 2 seconds with some info in the file. No thread names, the names are like (LWP 6012).

Btw, I can't reproduce the bug now, but I'm seeing a different behavior and maybe a clue to the issue.

It looks like when your mailbox is already synced for offline use (sync remote mail locally) and you receive new small mails, the notification of new mails works well. 
But when you receive a large mail like a few MB, Evo gets blocked of receiving server change notification until that mail is downloaded. So it looks like downloading new messages or messages for offline use blocks Evo from receiving new mail notifications.

When EWS is used to access Exchange account (512 MB mailbox, maybe 250MB used)
1) "downloading messages for offline use" takes ages. I have to leave it overnight to download
2) likewise downloading a new but large message (ie 5MB, 10 MB) literally takes ages. 

Downloading same message with IMAP using Thunderbird (I can also try with Evo if you like) takes  even 30 times less time. 

For example, the 10 MB email that blocks Evo from receiving notifications of new mails is still downloading since 12 minutes now. In this timeframe, I received new mails available on TB IMAP and mobile Activesync, but Evo doesn't yet show them. It's blocked in with "Downloading new messages for offline use" message on its status bar. 
Thunderbird has already downloaded the same mail in maybe 20 seconds.

Shall I open new bug report for the above issue?

OK, the 10 MB sized 1 new mail just finished downloading after 14 minutes of downloading, and 7 new messages popped up. One of those new mails is again 10MB, so i will probably need to wait another 15 minutes to receive other new ones again....

The gdb command was meant to be run as the same user as that which runs evolution. As I suppose you do not run evolution as root, thus nether the gdb should be run as root.

We are still comparing different protocols, IMAP, ActiveSync and Exchange Web Services.I do not know what causes the slowness for you, but I'm aware of
bug 719475, which, with IMAP protocol by Exchange server+IMAPx in evolution resulted in a slow download. Maybe this is the same issue, I do not know for sure, but if all what you see is related to slow downloads of large messages, and the notifications begin to work again after the large messages are downloaded, then the problem is the slowness, thus I'd rather use another bug report and don't fill this one.