Skip to content
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

Chat: Handle the new "duplicate" message error type, by not displaying it #7879

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cjb
Copy link
Contributor

@cjb cjb commented Jul 25, 2017

@keybase/react-hackers CC @chrisnojima @mmaxim

If we get a message with error type "duplicate". it means it was rejected by Gregor as a duplicate attempted send. Normally we display errors for failed messages and offer to retry them, but that's not appropriate here -- we don't want to display anything at all.

This PR seems to work, but I don't much like the way it turned out. We can learn of "duplicate" error messages in three different ways:

  1. They can be relayed to us from the outbox as part of a GetThread call, in which case we need to not render them at all.

  2. Race outcome 1: we can send a message, display it in pending state in the GUI, and then get an IncomingMessage notification telling us about the duplicate failure, in which case we need to delete it from the GUI after it's already been rendered.

  3. Race outcome 2: we can send a message, but due to a race between sagas and RPCs that RPCs win, we learn of the message's duplicate failure before we finish thinking we've sent a message. In this case we need to store away the fact that we've been told of a failure, and the fact that it's this special kind of failure, and then when we would normally render the message as pending in the GUI we decide not to.

To achieve this, I went with:

  1. When we unbox a message with this duplicate error, we store it as a messageKind of 'errorInvisible' instead of 'outboxText'. We have to store some kind of message, because it's an invariant in all of our code that an _unboxedToMessage() call produces a Message object for the store in every case.

  2. When we're rendering a thread, we now render these 'errorInvisible' messages an an empty <Box />.

  3. When we get an IncomingMessage telling us about a failure (Race outcome 1), we update the Message object in the store to have the messageKind updated to errorInvisible at that point, and it's rendered away as it would be in the first case.

  4. When we get an IncomingMessage telling us about a failure we haven't rendered yet (Race outcome 2), we just don't add anything at all in the store.

This is complicated enough that I'm not confident the code's going to be correct, and in general debugging these errors and races in transitioning messages from pending -> sent -> deleted is causing us a lot of hassle, so I don't feel good about merging this PR yet. We might be better just asking for CORE help to take care of this case. Or, we could try to refactor our code to eliminate this RPC<->sagas race, which I think would make us feel happier about adding new edge cases to this code.

What do you think?

@cjb cjb added the frontend label Jul 25, 2017
@mmaxim
Copy link
Contributor

mmaxim commented Jul 25, 2017

We can easily filter these from GetThread, in fact that probably is the correct behavior, I just didn't think to put it in at the time. I can make a commit to this branch to do that today.

@cjb
Copy link
Contributor Author

cjb commented Jul 25, 2017

I think my preferred fix for the race would be an exclusive lock shared between postMessage and incomingMessage -- if incomingMessage fires and there's already a postMessage in-progress, it should wait for postMessage to finish before doing work. That would avoid the race completely. But I'm not sure how expressable it is in Redux.

@mmaxim
Copy link
Contributor

mmaxim commented Jul 25, 2017

K, I'm still going to make the service change though, don't see any value of GetThread returning those.

payload.state.error &&
payload.state.error.typ === ChatTypes.LocalOutboxErrorType.duplicate
) {
messageKey = 'errorInvisible'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a message kind, not a message key

@MarcoPolo
Copy link
Contributor

Thinking about this after working on: #8016

I think this is an argument for us not trying to modify things we're getting in another saga. Because we can get into these nasty races.

I think in the context of #8016 to fix this you could:

  1. Turn this error into a delete message - example. Then no message with that id will be rendered.

It might make more sense to integrate this into that branch if we decide to move forward there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants