-
Notifications
You must be signed in to change notification settings - Fork 283
Add withTimeout to Chat and Offline Messages #1455
Conversation
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.
LGTM
cc: @drwasho @placer14 @cpacia please take a closer look at this as I added some more timeouts to offline messaging. The reason for this is that mobile is also having trouble with offline timeouts related to purchases. I've tested this locally on desktop and it seems to be performing as intended, but I'm not sure if you guys will find side effects this may impact. Below are two purchase calls that are hitting the 5s limit (modified the start for desktop to use 5s) and falling through. |
core/net.go
Outdated
@@ -96,7 +96,7 @@ func (n *OpenBazaarNode) SendOfflineMessage(p peer.ID, k *libp2p.PubKey, m *pb.M | |||
log.Debugf("Sending offline message to: %s, Message Type: %s, PointerID: %s, Location: %s", p.Pretty(), m.MessageType.String(), pointer.Cid.String(), pointer.Value.Addrs[0].String()) | |||
OfflineMessageWaitGroup.Add(2) | |||
go func() { | |||
ctx, cancel := context.WithCancel(context.Background()) | |||
ctx, cancel := context.WithTimeout(context.Background(), n.OfflineMessageFailoverTimeout) |
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.
Since this is in a separate goroutine I don't think we need a timeout here.
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.
Are we good on the other ones?
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 think the other ones are OK. If you remove this one we can merge.
007813e
to
b3c64d0
Compare
GTG @cpacia |
Helps fix #1224