Page MenuHomePhabricator
Create Task
Maniphest T6713

Adding a comment to a conpherence sometimes duplicates the message in the UI
Closed, ResolvedPublic
Actions

Assigned To
btrahan
Authored By
marpidone
Dec 8 2014, 11:14 PM
Tags
Referenced Files
F488257: dup.png
Jun 8 2015, 4:47 PM
F488293: dup2
Jun 8 2015, 4:47 PM
F247304: example.png
Dec 9 2014, 5:22 PM
F246833: 2014-12-08_17-46-40.png
Dec 8 2014, 11:14 PM
F246837: screenshot.png
Dec 8 2014, 11:14 PM
Subscribers
btrahan
chad
cspeckmim
epriestley
hach-que
marpidone
richardvanvelzen
• sshannin

Description

This has happened to me a couple times. For example, earlier today I tried to add a comment with a meme image and got this:

2014-12-08_17-46-40.png (860×718 px, 542 KB)

If I immediately refresh the page the duplicate doesn't exist.

A good way to reproduce seems to be to add a new user to an existing conpherence with content. The message that appears almost always causes quite a bit of duplication. For example:

screenshot.png (487×589 px, 30 KB)

I am using Firefox 33.1 on Windows 7, and a colleague was using the lastest Firefox on OSX (not sure the exact version) and see's the same issue.

Revisions and Commits

rP Phabricator
ClosedD12755 Conpherence - improve reliability of message delivery
ClosedD11143 Rewrite Aphlict to use Websockets
D13214rP818fcf00350a Conpherence - fix durable column message duplicating bug
D12819rP6c049d06ce4d Conpherence - change message rendering logic to eradicate possibility of…
D12092rP98899c822eb9 Conpherence - kill race conditions around update
D11277rP9a8eb4026e42 Make race condition window for Conpherence smaller and rarer
D10952rPd5e7cd5590e9 Conpherence - fix add participant / remove participant form

Related Objects
Search...

Event Timeline

marpidone created this task.Dec 8 2014, 11:14 PM
marpidone raised the priority of this task from to Needs Triage.
marpidone updated the task description. (Show Details)
marpidone added a project: Conpherence.
marpidone added a subscriber: marpidone.
chad triaged this task as Normal priority.Dec 8 2014, 11:59 PM
chad added a project: Transactions.
chad added a subscriber: btrahan.
chad added a subscriber: chad.

I see this often too

btrahan added a comment.Dec 9 2014, 12:17 AM

iiam

richardvanvelzen added a subscriber: richardvanvelzen.Dec 9 2014, 10:43 AM
btrahan added a comment.Dec 9 2014, 4:42 PM

So for folks experiencing this, the "add participants" codepath is different than the "adding another message" codepath. Its highly likely that fixing one does not fix the other.

As such, specific reproduction steps on the "adding another message" way this reproduces are very useful...! Please include them if you have them. Thanks!

epriestley added a subscriber: epriestley.Dec 9 2014, 4:45 PM

This is probably my fault, but I haven't actually seen it occur.

chad added a comment.Dec 9 2014, 4:46 PM

I have, I presume it to be some race condition with aphlict server / crappy internet connection.

marpidone added a comment.Dec 9 2014, 5:22 PM

Here's some exact steps that I just tested:

  1. Create a new message with myself as the only participant. Send the following messages: "Test", "Test 2", "Test 3", "Test 4".
  2. Refresh the page by hitting F5
  3. Add another participant (cspeck in this case)
  4. Refresh the page by hitting F5

This screenshot shows the results of these steps:

example.png (620×1 px, 69 KB)

Also, we are using phabricator on a LAN server, so internet connection may not be a factor. I could probably grab a copy of any relevant logs if they would help.

chad added a comment.Dec 9 2014, 5:31 PM

Do you run the aphlict server for live updates?

cspeckmim added a subscriber: cspeckmim.EditedDec 9 2014, 5:34 PM

We do run the aphlict service for live updates.
edit: just to clarify, @marpidone and myself are working together and testing out phabricator for our purposes.

btrahan added a revision: D10952: Conpherence - fix add participant / remove participant form.Dec 9 2014, 6:46 PM
btrahan added a commit: rPd5e7cd5590e9: Conpherence - fix add participant / remove participant form.Dec 9 2014, 7:29 PM
epriestley added a parent task: T5364: Conpherence v2 (Original Discussion).Jan 6 2015, 1:32 AM
cspeckmim added a comment.Jan 6 2015, 3:55 PM

This looks to no longer happen in the use-case of adding users to conpherences. I just added several users to a conpherence and the content wasn't duplicated. The server gets updated regularly so it would've been running from a revision from last night. We haven't been regularly using conpherences so it could have been fixed within the past few weeks.

epriestley added a comment.Jan 7 2015, 2:05 PM

I have a reliable repro case for this after D11143. Locally, the WebSocket notification channel is dramatically faster than the flash stuff was, and consistently causes a race condition:

  • Submit chat.
  • Get a notification about it before I get the response back from the server.
  • Since the notification happens first, we trigger an update.
  • Then the response comes back from the server and we add a copy of my message.
  • Then the update comes back and we add a second copy.

I imagine this could race in similar situations with multiple users, but this one is pretty easy for me to repro (all chat repros).

Fixes might involve:

  • Keep track of which transactionPHIDs we know about?
  • Do another "what ID do we know about?" check after responses come back but before we update the thread.
chad added a comment.Jan 7 2015, 3:01 PM

yay

epriestley added a revision: D11143: Rewrite Aphlict to use Websockets.Jan 8 2015, 7:17 AM
epriestley added a revision: D11277: Make race condition window for Conpherence smaller and rarer.Jan 8 2015, 8:38 AM
epriestley added a commit: rP9a8eb4026e42: Make race condition window for Conpherence smaller and rarer.Jan 8 2015, 5:44 PM
epriestley mentioned this in rP9e0f70e17d56: Rewrite Aphlict to use Websockets.Jan 8 2015, 6:03 PM
chad added a comment.Mar 15 2015, 10:34 PM

I haven't seen this issue in a while, do we feel it's resolved?

epriestley added a comment.Mar 15 2015, 10:40 PM

There's still a race condition that we should fix, the window is just a lot narrower.

epriestley added a parent task: T7571: Conpherence v2 Errata.Mar 16 2015, 7:12 PM
epriestley removed a parent task: T5364: Conpherence v2 (Original Discussion).Mar 16 2015, 7:14 PM
epriestley moved this task from Backlog to v2 on the Conpherence board.Mar 16 2015, 7:22 PM
btrahan claimed this task.Mar 16 2015, 7:36 PM
btrahan added a revision: D12092: Conpherence - kill race conditions around update.Mar 16 2015, 11:19 PM
btrahan closed this task as Resolved by committing rP98899c822eb9: Conpherence - kill race conditions around update.Mar 16 2015, 11:35 PM
btrahan added a commit: rP98899c822eb9: Conpherence - kill race conditions around update.
btrahan reopened this task as Open.Mar 17 2015, 11:53 PM
btrahan mentioned this in D12104: Conpherence - fix a typo.Mar 17 2015, 11:54 PM
btrahan added a comment.Mar 18 2015, 12:02 AM

This was fixed but D12104 breaks things again as the fix here was making basic updates a little wonky. I'll take another crack at fixing this. Code gets tighter each pass at least. :D

epriestley mentioned this in T7755: Improve reliability of real-time notification delivery.Apr 4 2015, 12:48 PM
btrahan mentioned this in T7571: Conpherence v2 Errata.May 1 2015, 10:28 PM
btrahan added a revision: D12755: Conpherence - improve reliability of message delivery.May 7 2015, 6:16 PM
btrahan mentioned this in rP791e897c0d0f: Conpherence - improve reliability of message delivery.May 7 2015, 6:27 PM
btrahan closed this task as Resolved.May 7 2015, 7:50 PM

I think I fixed this too with the recent changes. If not, how can I reproduce?

btrahan reopened this task as Open.May 11 2015, 6:11 PM
btrahan raised the priority of this task from Normal to High.
btrahan merged a task: T8141: Conpherence sidebar doubles up on messages.
btrahan added a subscriber: hach-que.

Current repro for durable column

  • open durable column
  • spam it
  • see dupes

I think I just need to make this transaction aware and render messages per transaction. Keeping track of updates that are in flight and adjusting as necessary hasn't proven to be a winning strategy for me so far.

btrahan added a revision: D12819: Conpherence - change message rendering logic to eradicate possibility of duplicates.May 12 2015, 11:20 PM
btrahan closed this task as Resolved by committing rP6c049d06ce4d: Conpherence - change message rendering logic to eradicate possibility of….May 13 2015, 6:06 PM
btrahan added a commit: rP6c049d06ce4d: Conpherence - change message rendering logic to eradicate possibility of….
• sshannin added a subscriber: • sshannin.Jun 8 2015, 1:12 PM

I still occasionally see messages being duplicated. I updated this past weekend.

• sshannin reopened this task as Open.Jun 8 2015, 1:23 PM

Reopening in case that sets off Herald for some people.

btrahan closed this task as Resolved.Jun 8 2015, 4:08 PM

Can you please re-open with reproduction instructions? Algorithmically speaking, this is impossible AFAIK because we now use the unique transaction ids to guarantee we only render each transaction once.

btrahan reopened this task as Open.Jun 8 2015, 4:40 PM

@sshannin is sending me screenshots where this is occurring in the durable column view with asynchronous updates via other users sending messages so I'll take a poke around today in case there's something one off in that code path.

• sshannin added a comment.Jun 8 2015, 4:47 PM

To document

  • every message @btrahan sent me came in duplicated at first (first screenshot)
  • None of the message I sent showed as duplicated for me
  • Suspecting maybe some weird firefox behavior, I opened a chrome side-by-side
  • My cross-browser messages also showed duplicated (second screenshot)

This all was on secure.phab within the past few minutes.

dup.png (1×1 px, 450 KB)

dup2 (1×1 px, 461 KB)

btrahan added a revision: D13214: Conpherence - fix durable column message duplicating bug.Jun 8 2015, 9:11 PM
btrahan closed this task as Resolved by committing rP818fcf00350a: Conpherence - fix durable column message duplicating bug.Jun 8 2015, 9:12 PM
btrahan added a commit: rP818fcf00350a: Conpherence - fix durable column message duplicating bug.
btrahan added a comment.Jun 8 2015, 9:16 PM

Thanks for the persistence @sshannin. :D

If after updating to HEAD People are still experiencing issues please do re-open with reproduction instructions. Thanks!

• sshannin added a comment.Jun 8 2015, 9:26 PM

Much heart

Phabricator · Built by Phacility