Implement precise, unbatched notification of sync completion events #1054
Conversation
This avoids a race condition where an earlier upload completion event will notify a later waiter whose changes haven't been uploaded yet.
| auto waiter = is_download ? &sync::Session::async_wait_for_download_completion | ||
| : &sync::Session::async_wait_for_upload_completion; | ||
| (m_session.get()->*waiter)([resync_counter, weak_self, is_download](std::error_code ec) { | ||
| (m_session.get()->*waiter)([is_download, | ||
| resync_counter = m_client_resync_counter, |
There was a problem hiding this comment.
I think the resync_counter logic is now redundant since if there was a resync, it won't find the entry for this id in the map. If you agree, I will remove that logic.
There was a problem hiding this comment.
Yes, this looks like a more general solution to the problem the resync counter was addressing.
| m_upload_completion_callbacks.push_back(std::move(callback)); | ||
| m_completion_request_counter++; // read in wait_for_completion() | ||
| m_state->wait_for_completion(*this, _impl::SyncProgressNotifier::NotifierType::upload); | ||
| m_upload_completion_callbacks.emplace_hint(m_upload_completion_callbacks.end(), |
There was a problem hiding this comment.
Do you think it is worth moving this into add_completion_callback? I can't really see any reason for the current distribution of work between the functions, but I didn't want to make too much unnecessary change in a bugfix commit.
There was a problem hiding this comment.
Do we still need separate upload/download completion callback maps with this change? If not then merging them and pushing this into add_completion_callback() makes sense, but without merging them it'd be another awkward conditional there?
There was a problem hiding this comment.
No, I don't think so. Merging them cleans up the code nicely.
There was a problem hiding this comment.
This doesn't handle completion handlers added while the state is Inactive correctly. In sync_session_states::Active::enter_state() it calls add_completion_callback() once per direction if there's any pending completion handlers, which no longer does the required thing.
I'm not sure if any of that code still actually makes sense. Pre-v10 it was primarily there to support adding completion handlers while waiting for the refresh token, and it might just be dead code now or only required for very strange things (adding a completion handler after calling logout and then expecting it to be called after logging in?).
| auto waiter = is_download ? &sync::Session::async_wait_for_download_completion | ||
| : &sync::Session::async_wait_for_upload_completion; | ||
| (m_session.get()->*waiter)([resync_counter, weak_self, is_download](std::error_code ec) { | ||
| (m_session.get()->*waiter)([is_download, | ||
| resync_counter = m_client_resync_counter, |
There was a problem hiding this comment.
Yes, this looks like a more general solution to the problem the resync counter was addressing.
| m_upload_completion_callbacks.push_back(std::move(callback)); | ||
| m_completion_request_counter++; // read in wait_for_completion() | ||
| m_state->wait_for_completion(*this, _impl::SyncProgressNotifier::NotifierType::upload); | ||
| m_upload_completion_callbacks.emplace_hint(m_upload_completion_callbacks.end(), |
There was a problem hiding this comment.
Do we still need separate upload/download completion callback maps with this change? If not then merging them and pushing this into add_completion_callback() makes sense, but without merging them it'd be another awkward conditional there?
|
@tgoyne, do you know what the status of this is? This PR doesn't currently compile - at least not on my machine. I looked at making the fixes/improvements in the comments between you and Mathias, but they cause a bunch of test failures. Before I dig into this anymore, do you know if this is still something we want to do? |
|
It is a bug that needs to be fixed still, and this is most of the way to a correct fix. I think the only thing left to do is to figure out if we still need to support adding completion handlers while the session is inactive, and then either removing that code (if we don't) or finding a way to make that work correctly. |
|
Closing this in favor of #1118. |
This avoids a race condition where an earlier upload completion event will
notify a later waiter whose changes haven't been uploaded yet.
Closes #1056.