Read timeouts in fsmpico #4
Open
Conversation
This file contains hidden or 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
This commit is a transfer of GitLab branch 88-read-timeouts-in-fsm.
dettonijr
requested changes
Jan 31, 2018
| @@ -69,6 +69,8 @@ | |||
|
|
|||
| // Defines | |||
|
|
|||
| #define READ_TIMEOUT (5000) | |||
| #define CONTINUOUS_TIMEOUT_LEEWAY (1000) | |||
There was a problem hiding this comment.
Can we unify those two constants in a single leeway? They seem to have similar semantics.
src/fsmpico.c
Outdated
| LOG(LOG_DEBUG, "Reconnecting for continuous authentication"); | ||
| fsmpico->comms->reconnect(fsmpico->user_data); | ||
| break; | ||
| case FSMPICOSTATE_PICOREAUTH: | ||
| createMessagePicoReauth(fsmpico, message, extraData); | ||
| fsmpico->comms->write(buffer_get_buffer(message), buffer_get_pos(message), fsmpico->user_data); | ||
| stateTransition(fsmpico, FSMPICOSTATE_SERVICEREAUTH); | ||
| // set a timeout for awaiting a response | ||
| fsmpico->comms->setTimeout(fsmpico->reauthDelay + CONTINUOUS_TIMEOUT_LEEWAY, fsmpico->user_data); |
There was a problem hiding this comment.
There is some inconsistence in this timeout.
Service sends reauth 10 seconds
Pico replies back and set a timeout to 10s + LEEWAY
Service sends reauth 10 seconds
Pico waits for 10 seconds
Pico replies back and set a timeout to 10s + LEEWAY
The timeouts should expire in the same point in time.
Remove the timeout between reading a ServiceReauth message and sending the PicoReauth message.
llewelld
reviewed
Feb 6, 2018
| @@ -364,6 +372,8 @@ void fsmpico_read(FsmPico * fsmpico, char const * data, size_t length) { | |||
| createMessagePicoAuth(fsmpico, message, fsmpico->extraData); | |||
| fsmpico->comms->write(buffer_get_buffer(message), buffer_get_pos(message), fsmpico->user_data); | |||
| stateTransition(fsmpico, FSMPICOSTATE_STATUS); | |||
| // set a timeout for waiting for the status message | |||
| fsmpico->comms->setTimeout(READ_TIMEOUT, fsmpico->user_data); | |||
There was a problem hiding this comment.
To match the pattern of fsmservice, the timeout on connect is sufficient. This timeout should be removed, unless there's a good reason for it that I'm missing?
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.
Transfer of merge request !90 from GitLab.