refactor: Made PendingEventsStore interface asynchronous#513
refactor: Made PendingEventsStore interface asynchronous#513zashraf1985 wants to merge 6 commits into
Conversation
| // assert that the passed in callback to pendingEventsDispatcher was called | ||
| expect(callback).toHaveBeenCalledTimes(1) | ||
| expect(callback).toHaveBeenCalledWith({ statusCode: 200 }) | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Why did you add this setTimeout?
There was a problem hiding this comment.
Because i want to detect call dispatchEvent on the original event dispatcher which was called synchronously earlier but now its being called asynchronously after the set promise resolves.
| try { | ||
| this.send(item, () => {}) | ||
| } catch (e) {} | ||
| this.store.values().then((pendingEvents) => { |
There was a problem hiding this comment.
- Did you test this in the browser to see how its behavior compares to the old version? I'm curious if the additional async step would affect how much of this is allowed by the browser to execute during a page close.
- If
closeis called whilevalues()is in progress, I think we need to stop and not send the events. To the extent possible, we should stop all additional work aftercloseis called.
| this.dispatcher.dispatchEvent(entry.request, response => { | ||
| this.store.remove(entry.uuid) | ||
| callback(response) | ||
| this.store.set(entry.uuid, entry).then(() => { |
There was a problem hiding this comment.
Same comment as above RE: doing things after close: If close is called while set(...) is in progress, we need to stop and not send the events.
There was a problem hiding this comment.
Discussed this offline w/Zeeshan - this part is OK as-is, disregard.
| callback(response) | ||
| this.store.set(entry.uuid, entry).then(() => { | ||
| this.dispatcher.dispatchEvent(entry.request, response => { | ||
| this.store.remove(entry.uuid) |
There was a problem hiding this comment.
I think we need to wait for remove to complete before calling the callback. This callback eventually feeds into the close promise status. We don't want close to resolve while an async operation is still pending.
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
I see event ordering issues in some places. All the events queued must be sent to the server in their dispatch orders. See my comments.
| logger.debug('Sending %s pending events from previous page', pendingEvents.length) | ||
| pendingEvents.forEach(item => { | ||
| try { | ||
| this.send(item, () => {}) |
There was a problem hiding this comment.
Does MAP guarantee the fifo order in JS? Otherwise, we lose the order of events dispatched.
There was a problem hiding this comment.
Also, orders are concerned in other cases as well:
- if we have any queued events, all the following events dispatched should be queued as well instead of sending them first. We may have queued events from the previous session or new events queued from the last failed dispatch.
| callback(response) | ||
| this.store.set(entry.uuid, entry).then(() => { | ||
| this.dispatcher.dispatchEvent(entry.request, response => { | ||
| this.store.remove(entry.uuid).then(() => callback(response)) |
There was a problem hiding this comment.
We also need a protection from one packet stuck in the queue forever (cannot send to the server successfully for JSON format error etc). This can be an issue when we change them in FIFO order.
|
This is no longer needed now. I am now leaving the existing localstorge implementation intact and implementing event caching for react native separately in a new PR. |
4 participants
Summary
Made PendingEventsStore interface asynchronous. This change is done to implement React Native Async Storage which is asynchronous using the same interface.
Test plan
Fixed the failing unit tests