Page MenuHomePhabricator
Differential D14188

[lib] Fix missing thread bug in updateRelationshipsAndSendRobotext()
AcceptedPublic
Actions

Authored by angelika on Wed, Jan 8, 5:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jan 9, 8:57 AM
Unknown Object (File)
Thu, Jan 9, 8:52 AM
Unknown Object (File)
Thu, Jan 9, 8:33 AM
Unknown Object (File)
Thu, Jan 9, 8:30 AM
Unknown Object (File)
Thu, Jan 9, 8:23 AM
Unknown Object (File)
Thu, Jan 9, 8:17 AM
Unknown Object (File)
Thu, Jan 9, 7:52 AM
Unknown Object (File)
Thu, Jan 9, 7:52 AM
Subscribers
ashoat

Details

Reviewers
tomek
kamil
Summary

Fixes https://linear.app/comm/issue/ENG-9946/cannot-read-properties-of-undefined-reading-members

In createThickThreadAndSendRobotextPromise there are two called two functions: createNewThickThread and sendRobotextToThickThread. After the thread is created, sendRobotextToThickThread is called but it’s an outdated reference that was captured before createNewThickThread is called and it uses old threads storage without the new thread.
The fix is inspired by "step-based approach" in the codebase but modified so that it allows for waiting for multiple threads at once.

Depends on D14187 but just for testing

Test Plan

Go to Profile -> Friend list
Select 1 or more users and add them as friends
Click Save
Verify that the requests are sent and there is no longer an error in the logs about missing thread

Diff Detail

Repository
rCOMM Comm
Branch
graszka22/ENG-9946
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika created this revision.Wed, Jan 8, 5:35 PM
Herald added a subscriber: ashoat. · View Herald TranscriptWed, Jan 8, 5:35 PM
angelika edited the summary of this revision. (Show Details)Wed, Jan 8, 5:36 PM
angelika added a parent revision: D14187: [native] Fix adding several friends at once.
Harbormaster completed remote builds in B33205: Diff 46618.Wed, Jan 8, 5:53 PM
angelika requested review of this revision.Wed, Jan 8, 5:53 PM
tomek requested changes to this revision.Thu, Jan 9, 1:59 AM
tomek added inline comments.
lib/hooks/relationship-hooks.js
43–48

We're using it more like a queue or even a pool of operations - step indicates that we have to consume them one-by-one and we don't do that. To reduce confusion, we can rename it.

118–121

This looks really dangerous because it allows growing the steps indefinitely. We should only add a new step if it isn't already processed (so move this if after the next one).

125

We never set the value in a map to false, so maybe we can simplify it by replacing it with a Set.

260

We prefer using spread over using the concat.

This revision now requires changes to proceed.Thu, Jan 9, 1:59 AM
angelika added inline comments.Thu, Jan 9, 7:31 AM
lib/hooks/relationship-hooks.js
118–121

We're not adding any new steps to the state in this useEffect. The steps added to newSteps array are the steps that are already in steps array and are still waiting for the thread so the length of newSteps array is always <= the length of steps array.

angelika updated this revision to Diff 46634.Thu, Jan 9, 7:38 AM
angelika edited the summary of this revision. (Show Details)

Review changes

Harbormaster completed remote builds in B33215: Diff 46634.Thu, Jan 9, 7:56 AM
tomek requested changes to this revision.Fri, Jan 10, 1:53 AM

The changes look good, but I have some more questions, that probably should've been asked in the first iteration - sorry for that!

lib/hooks/relationship-hooks.js
120 ↗(On Diff #46634)

What if there are two operations for a single thread ID? Would we skip both of them when only one was in progress? Maybe we can keep a flag inside Operation telling if it's in progress. Or maybe we can use something unique as a key in the map?

130–132 ↗(On Diff #46634)

Is it possible that in one iteration, we add one operation and remove another? If that's the case, we should introduce a flag telling if the operations changed instead of relying on the length.

This revision now requires changes to proceed.Fri, Jan 10, 1:53 AM
angelika added inline comments.Fri, Jan 10, 5:31 AM
lib/hooks/relationship-hooks.js
120 ↗(On Diff #46634)

I think it's not possible for two operations to have the same thread ID. Notice that operations are added just in one place: when we just created a new thread and we need to wait for this thread to be in the store to send a message to it.

130–132 ↗(On Diff #46634)

As mentioned in my other comment: we're not adding any new operations to the state in this useEffect. For each operation in the operations array we're either:

  • processing the operation and not adding it to newOperations, so removing operation from further processing
  • adding the operation to newOperation, so just retaining the operation that was in operations array before

so the length of newOperations array is always <= the length of operations array. And if there are less operations in newOperations array than in operations array it means that we removed some operations.

Maybe the name newOperations is misleading? Maybe something like unprocessedOperations or retainedOperations would be better?

tomek accepted this revision.Fri, Jan 10, 5:56 AM
tomek added inline comments.
lib/hooks/relationship-hooks.js
120 ↗(On Diff #46634)

Ok, this makes sense.

130–132 ↗(On Diff #46634)

Ah, ok, you're right! I think that retainedOperations might be a good idea.

This revision is now accepted and ready to land.Fri, Jan 10, 5:56 AM

Revision Contents
Changeset List

PathSize
lib/
hooks/
67 lines

Diff 46618

View Options

lib/hooks/relationship-hooks.js

Loading...