-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed messages are repeated on page refresh & process queue only when client is leader #2703
fixed messages are repeated on page refresh & process queue only when client is leader #2703
Conversation
let isInit = false; | ||
|
||
// Are we ready to determine active Client? | ||
const isReady = new Promise((res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are needed as the Client manager incorrectly reports the leadership of the client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you maybe elaborate on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure. There are cases where we run important callbacks once but It is possible that the client is not set before that callback call. Thus a promise to make sure that we run the callback. We have to use a flag and combination of Promises to achieve this.
as
function init() {
Onyx.merge(ONYXKEYS.ACTIVE_CLIENTS, [clientID]);
isInit = true;
}
The above method does not really initialize the client it just request Onyx to set the client id. merge
Ops is async and I think it does not return thenable callback thus we have to go to connect
call.
if (isInit) {
setReady();
}
Now we mark is ready by resolving the promise as we have set the clientId on the client stack. This connect could fire multiple times, we only resolve the promise when isInit
is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to wait until the client is set to do the rest of the logic here?
One idea would be to add an onReady
function to ActiveClientsManager
so that we can register a callback then wait to do the Onyx.connect()
below?
I think it might make it more obvious that we are waiting for the active client to be set
ActiveClientsManager.onReady(() => {
Onyx.connect({
key: ONYXKEYS.NETWORK_REQUEST_QUEUE,
callback: ...
})
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok Thanks I am seeing the problem now. And yeah, Onyx being asynchronous is making this difficult once again 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @parasharrajat, Can we try to document this code better and add some comments? Overall, I'm confused by the implementation and unsure why we're doing most of what we're doing.
let isInit = false; | ||
|
||
// Are we ready to determine active Client? | ||
const isReady = new Promise((res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you maybe elaborate on this?
src/libs/Network.js
Outdated
|
||
let isQueuePaused = false; | ||
|
||
// Are we retrying requests when user comes back online? | ||
let retryRequestsAdded = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above doesn't really help explain what the code is doing or how it works.
src/libs/Network.js
Outdated
return; | ||
} | ||
ActiveClientManager.isReady.then(() => { | ||
// Only process queue when client is Leader and we have not retried the requests already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we only process the queue when the client is the leader? (Put it in the comment)
src/libs/Network.js
Outdated
@@ -112,6 +122,7 @@ function processNetworkRequestQueue() { | |||
!request.data.doNotRetry && request.data.persist | |||
)); | |||
Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, retryableRequests); | |||
retryRequestsAdded = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcaaron. We have a queue of requests as networkRequestQueue
. we take retry requests and save them in ONYXKEYS.NETWORK_REQUEST_QUEUE
in case the user closes the tab or refreshes it. Now let's say the user's internet is back but he didn't refresh the tab so networkRequestQueue
will continue processing all those requests. This flag records this behavior. so when this is true
we know we have processed the requests and clear the requests.
// User came back online and the request queue will resume, thus we should clear the NETWORK_REQUEST_QUEUE
if (ActiveClientManager.isClientTheLeader() && retryRequestsAdded) {
Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, []);
}
Also Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, retryableRequests);
. We reset the queue here but what will happen if the user is sending requests from multiple tabs. We will overwrite the old requests. Do we want this to behave like this or we should merge those requests Onyx.merge
?
src/libs/Network.js
Outdated
@@ -160,6 +171,11 @@ function processNetworkRequestQueue() { | |||
.catch(error => onError(queuedRequest, error)); | |||
}); | |||
|
|||
// User came back online and the request queue will resume, thus we should clear the NETWORK_REQUEST_QUEUE | |||
if (ActiveClientManager.isClientTheLeader() && retryRequestsAdded) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a little lost on why we only want to clear the queue when the client is the leader and there is also no explanation about the significance of retryRequestsAdded
. Can you try explaining this better in the comments?
// User came back online
How exactly does this tell us the user came back online?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So When a user goes offline we filter the network request queue and put the retrievable requests on persisted Queue. But it is possible that a user can come back online without refreshing the page. Now those retriable requests will process as a part of networkRequestQueue
. Thus we don't want to process the persisted queue ONYXKEYS.NETWORK_REQUEST_QUEUE
. It should only run when the user refresh the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only allow the leader to handle the ONYXKEYS.NETWORK_REQUEST_QUEUE
as persisted requests shared by tabs and make sure that we only clear the queue when they were processed via normal networkRequestQueue
. As we have processed the normal queue, now we will not need them to be persisted thus clear them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's still a little unclear to me, but I might have to just look harder at this and try to offer some better suggestions.
I will comment on everything as requested. |
let isInit = false; | ||
|
||
// Are we ready to determine active Client? | ||
const isReady = new Promise((res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to wait until the client is set to do the rest of the logic here?
One idea would be to add an onReady
function to ActiveClientsManager
so that we can register a callback then wait to do the Onyx.connect()
below?
I think it might make it more obvious that we are waiting for the active client to be set
ActiveClientsManager.onReady(() => {
Onyx.connect({
key: ONYXKEYS.NETWORK_REQUEST_QUEUE,
callback: ...
})
});
let setReady = null; | ||
|
||
// Whether the client Manager has started | ||
let isInit = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead could we have something like:
shouldFireOnReadyCallback
and didInitialize
and then do
if (shouldFireOnReadyCallback && !didInitialize) {
onReadyCallback();
didInitialize = true;
}
let isInit = false; | ||
|
||
// Are we ready to determine active Client? | ||
const isReady = new Promise((res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok Thanks I am seeing the problem now. And yeah, Onyx being asynchronous is making this difficult once again 😓
src/libs/Network.js
Outdated
@@ -160,6 +171,11 @@ function processNetworkRequestQueue() { | |||
.catch(error => onError(queuedRequest, error)); | |||
}); | |||
|
|||
// User came back online and the request queue will resume, thus we should clear the NETWORK_REQUEST_QUEUE | |||
if (ActiveClientManager.isClientTheLeader() && retryRequestsAdded) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's still a little unclear to me, but I might have to just look harder at this and try to offer some better suggestions.
…asharrajat/offline
a971bad
to
ed60b40
Compare
ed60b40
to
8289bad
Compare
@marcaaron So I found one more side effect so I changed the logic completely. Now the code completely rely on persisted queue. let me know what do you think of it. I would be happy to explain everything. Due to Onyx.connect is called when user comes back online, we can just use the persisted queue and new logic is build up on that. |
* | ||
* @returns {Promise<void>} | ||
*/ | ||
function isReady() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we don't need to keep the promise in memory. I used Promise over your suggested callback method onReady(() => {})
. We can just use promise in multiple places for multiple tasks. The callback would require an array to be implemented for that purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I want to do it the way I've suggested for consistency reasons. It's not that your idea won't work, it's that we don't have many patterns like this and it's pretty unusual to see.
We can just use promise in multiple places for multiple tasks. The callback would require an array to be implemented for that purpose.
There is no need to do this as only one thing needs to know whether we have "initialized".
* | ||
* @returns {Promise<void>} | ||
*/ | ||
function isReady() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I want to do it the way I've suggested for consistency reasons. It's not that your idea won't work, it's that we don't have many patterns like this and it's pretty unusual to see.
We can just use promise in multiple places for multiple tasks. The callback would require an array to be implemented for that purpose.
There is no need to do this as only one thing needs to know whether we have "initialized".
src/libs/Network.js
Outdated
if (didLoadPersistedRequests || !persistedRequests) { | ||
return; | ||
} | ||
ActiveClientManager.isReady().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I will repeat that I would prefer that we not do the check this way.
|
||
// If we have a request then we need to check if it can be persisted in case we close the tab while offline. | ||
// We filter persisted requests from the normal Queue to remove duplicates | ||
networkRequestQueue = _.reject(networkRequestQueue, (request) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We filter persisted requests from the normal Queue to remove duplicates
Which duplicates are we talking about here? Can you provide an example? It's hard to visualize what you mean or why we are removing these from the normal request queue in memory.
Wouldn't this mean that if I add a comment while offline that it will be "removed" from the queue and not sent again when I come back online?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Onyx.connect is called for NETWORK_REQUEST_QUEUE
when we come back online which takes care of those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link me to where this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a user comes online on line 37 we process the queue by fake merge which I will optimize based on the suggestion.
Sorry. Posting it here. I found the following issues.
What the code is doing? OLD Architecture####Single Tab(Client) case When a user is offline and we get requests, We take retry requests from the normal queue and push them to Persisted Queue(We have to). We keep the copy of those requests in the normal queue as well. Thus when we get back online. requests will be sent from Persisted Queue & normal queue.(duplicates) Multi Client CaseWe get requests while offline, Single Tab(Client) case will be repeated for all tabs. But Persisted Queue is shared among tabs so we have multi-tab duplicated requests now. With respect to this, due to when we push to Persisted queue. Onyx.connect will be called and it will push the requests to each tabs normal queue for the first time then So Recent changes change the architecture in the following way Proposed Architecture
OR
I think it is a simpler approach and more cleaner but I could be missing something. Otherwise, I don't think fixing the linked issue considering single tab behavior is good it would be temporary. We can wait until we are looking at the whole scenario. |
Hey @parasharrajat can you please summarize the status of this PR and linked issue? Thanks. |
I will look into again and try to cross check everything. Also I will update the Promise to what you have suggested. Overall, my solution would be based on the explanation above. |
gentle bump |
Thanks for the Bump. I will try to check it off from my list this weekend. I need to rework here as there have been numerous changes since the PR was created. |
@joelbettner I have updated the PR and tested it. For the first case, we need to fire the callback as soon as we register. thus it will require another flag. I believe that the promise-based approach is neater and it allows multiple handlers. Let me know if you want me to do it in a callback way. Thanks. Also I commented the code. Let me know if some thing is not clear. There is small explanation in the issue description as well. |
Just leaving a comment that I won't be able to help out with this review since there are higher priority items for me to get to at the moment. If you need help please ask in the Slack channel. Also curious if @TomatoToaster wants to check this PR out since he recently edited these files and might understand how this stuff works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi I just had some questions for clarifying. Just making sure I understand how this works correctly.
Hi @TomatoToaster, I will get back to you with answers on these But I am sure about that flag that it is needed. |
Does anyone feel that my solution is worth merging by doing some changes and refactor, then please do let me know? otherwise, this PR can be closed. Thanks. |
I'll give this another look tomorrow and let you know if I have any
additional feedback!
…On Mon, Aug 2, 2021 at 2:57 PM Rajat Parashar ***@***.***> wrote:
Does anyone feel that my solution is worth merging by doing some changes
and refactor, then please do let me know? otherwise, this PR can be closed.
Thanks.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2703 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB67VTNMGWXFJKZ5KMTT24BEZANCNFSM44FUNWSA>
.
|
@tgolen any thoughts? |
Nope, I still have other things on my list to get to before I look at this today. Thanks for your patience! |
src/libs/Network.js
Outdated
// b) User is online. | ||
// c) requests are not already loaded, | ||
// d) When there is at least one request | ||
if (!ActiveClientManager.isClientTheLeader() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still a big fan of this suggestion based on the following arguments:
- There is no hint anywhere that this method must be run after
ActiveClientManager.isReady()
- If
isClientTheLeader()
is ever called without.isReady()
then it's always going to give a false result
This is setting a trap for anyone else that decides to use isClientTheLeader()
in the future.
|
||
// If we have a request then we need to check if it can be persisted in case we close the tab while offline. | ||
// We filter persisted requests from the normal Queue to remove duplicates | ||
networkRequestQueue = _.reject(networkRequestQueue, (request) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
It is not correct. Only on page reload this gives the wrong result (Until Onyx sets the key). for later cases it works just fine.
Sorry I didn't understand.
Same for this.
Sure I will do that. |
OK, thanks for explaining. I see I wasn't completely correct, but the trap is still there. For example: if I add code somewhere else that executes on page load, and only reference
Oh, sorry! "bump" is a common term we use that indicates there is an unanswered question that needs to be addressed.
Thanks! |
Updated @tgolen. |
This is ready for another review. I can move to the callback-based ATM this PR fixes two issues. Not sure of the prioirty. |
🙄 Others please share your valuable feedback. |
Sorry, I am being impatient here but see no reason to hold unless you tell me the reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making that change to the callbacks.
We are not really seeing eye-to-eye on isClientTheLeader()
being asynchronous.
If @TomatoToaster and @joelbettner are absolutely fine with the current implementation, and they feel they understand it and could fix any race conditions with it (as opposed to just approving this PR because of review-fatigue), then I am OK merging this as-is.
All yours @marcaaron |
I agree with the point. Just finding it difficult to handle in the ONYX.connect callback without too much boilerplate. I am ready to move forward with the current implementation as ClientManager usages do not have any such race condition. Maybe if we have any problems with this we can rethink the approach here. |
I have to be honest... reviewing this is going to be a bit of a context switch for me at the moment. The changes still seem a bit hard to follow, but to be fair, the persisting of requests did not really fit cleanly into the design when I added it. That said, I feel like it's very possible we will want to do something like a Phase 2 Refactor for the networking code. And that's probably a good time to think a bit more about how we want this to work and just do it right. So yeah that's a long way of saying... it feels like persisting requests are in a somewhat broken state and if this fixes them then I'm 👍 |
Thanks Marc. Not sure what is the next step. |
Yeah I think we can merge this. I can't think of a more perfect way to fix right now this and I agree that we can revisit this on a stage 2 refactor. I'll also sign up to help out on fixing any bugs with these persisted requests going forward 👍🏾 and help out with the eventual refactor since I've got this context at least. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @TomatoToaster in version: 1.0.85-10 🚀
|
This has been deployed to production and is now subject to a 7-day regression period. |
🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀
|
This has been deployed to production and is now subject to a 7-day regression period. |
Please review @marcaaron.
Details
Fixed Issues
Fixes #2451
Fixes #4214
Tests / QA Steps
Tested On
Screenshots
Web
Before
OFFLINE.mp4
After
new.mp4