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

[RTI-15742] Close connection after writing the last checkpoint #86

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

abijr
Copy link
Contributor

@abijr abijr commented Mar 1, 2024

Close connection after writing the last checkpoint when receiving a terminate event.

This PR adds a state_timeout (see gen_statem state-timeout), this timeout will be cancelled if there's a state change (which will happen if we get a response). If we want to trigger the timeout regardless of state changes, we can use a generic timeout.

The way this timeout works is by inserting the timeout trigger when setting state to {?PEER_READ, ?SHUTDOWN} so this timeout will protect us from staleness in both cases of terminate, {ok, Checkpoint}} and {_, ok} in the MLD shutdown handler. Because both success/3 and shutdown_checkpoint/2 set next state to {?PEER_WRITE, ?SHUTDOWN} or {?PEER_WRITE, ?SHUTDOWN_CHECKPOINT} with the {write, IoData} event.

UPDATE:

  • PR now only handles {?PEER_READ, ?SHUTDOWN}
  • We use generic timeout instead of state timeout, so timeout always triggers regardless of state changes.

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2024

CLA assistant check
All committers have signed the CLA.

src/erlmld_wrk_statem.erl Outdated Show resolved Hide resolved
@tak30
Copy link
Contributor

tak30 commented Mar 1, 2024

I would be in favour of using a generic timeout just to ensure we really close the socket

Copy link
Contributor

@raz-adroll raz-adroll left a comment

Choose a reason for hiding this comment

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

Also, can we log something if we receive the shutdown message?

src/erlmld_wrk_statem.erl Outdated Show resolved Hide resolved
@abijr abijr merged commit 964cd27 into main Mar 4, 2024
3 checks passed
@abijr abijr deleted the RTI-15742 branch March 4, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants