Better errors for ResponseHandler and cancel ACK and response waits on disconnect #64
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javiercr
commented
May 19, 2024
javiercr
commented
May 19, 2024
| @@ -21,7 +21,7 @@ func (c *Client) ExecuteTasks(tasks ...socket.Task) (*table.LSTable, error) { | |||
|
|
|||
| resp, err := c.socket.makeLSRequest(payload, 3) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("failed to send LS request: %v", err) | |||
| return nil, err | |||
There was a problem hiding this comment.
Heads up: this is gonna cause changes in the log files / records for users of the bridge, including Beeper infra. As the error strings will be different now.
javiercr
commented
May 19, 2024
| case <-time.After(packetTimeout): | ||
| p.deleteDetails(packetId, channelType) | ||
| // TODO this should probably be an error |
There was a problem hiding this comment.
I guess the TODO comment here was right :)
Co-authored-by: Tulir Asokan <tulir@maunium.net>
Co-authored-by: Tulir Asokan <tulir@maunium.net>
Co-authored-by: Tulir Asokan <tulir@maunium.net>
tulir
reviewed
May 20, 2024
tulir
reviewed
May 21, 2024
tulir
approved these changes
May 21, 2024
Base automatically changed from
feature/improve-sending-while-reconnecting
to
main
May 22, 2024 08:32
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
This PR continues the work on #63 (and should be merged after that one).
Description
Given the following sequence diagram for sending a Message:
If a message is sent by the user, right an instant before a connection refresh start, the message will be delivered by Meta after receiving our
PUBLISH (/ls_req)(1) packet.However the connection refresh could be closing the socket before Meta's
PUBACK(2), or thePUBLISH(/ls_resp)(3) are received by us.In that scenario, the
makeLSRequestwill be locked until the timeout defined inResponseHandler.waitForDetailsis triggered.This PR changes that behavior by exposing the
Contextused byClient, so that whenever that Context is cancelled (by callingstopCurrentConnection), we also notifyResponseHandler.waitForDetails.This PR also changes the signature for:
waitForPubACKDetails,waitForSubACKDetails, andwaitForPubResponseDetailsso that they return a different sentinel error depending on each scenario.TO BE DISCUSSED:
We need to discuss how to handle the new
ErrContextCancelledinPortal.handleMatrixMessage. With the current implementation in this PR, the sent message will still be flagged as "Not delivered" in Beeper, despite having been delivered properly.