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

Fix Session.Renew to care about 404s properly #1041

Merged
merged 1 commit into from
Sep 25, 2015
Merged

Fix Session.Renew to care about 404s properly #1041

merged 1 commit into from
Sep 25, 2015

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Jun 16, 2015

Fixes part of #1040.

Needs some sort of handling for how to deal with RenewPeriodic.

@rboyer
Copy link
Member Author

rboyer commented Jun 17, 2015

In a test program with a slightly modified RenewPeriodic() call, I had it print the SessionEntry and error return value every time it was called. I deliberately had the program take a very long sleep in between creating the session and attempting to renew it (it had delete behavior). This was to showcase what happens when the leader evaporates your session while you were trying to be a good renewing citizen (but something like a long GC pause or similar caused you to miss your renewal window):

2015/06/17 09:04:11 created session 85bb5662-4705-2e1b-4b7f-7997c5d2cd55 with ttl of 10s
2015/06/17 09:05:01 renewing session 85bb5662-4705-2e1b-4b7f-7997c5d2cd55
2015/06/17 09:05:01 got entry=[*api.SessionEntry/<nil>], err=[*errors.errorString/Unexpected response code: 404 (Session id '85bb5662-4705-2e1b-4b7f-7997c5d2cd55' not found)]
...this repeats several times...
2015/06/17 09:05:06 session died: 85bb5662-4705-2e1b-4b7f-7997c5d2cd55

It only dies because the full TTL elapsed. With the proposed changes on this MR:

2015/06/17 09:11:17 renewing session d8ecada8-ec39-8336-7eee-847a34d5a2f1
2015/06/17 09:11:17 got entry=[*api.SessionEntry/<nil>], err=[<nil>/<nil>]
2015/06/17 09:11:17 session died: d8ecada8-ec39-8336-7eee-847a34d5a2f1

It doesn't try to keep renewing the session that has evaporated (which is the sane thing to do).

@rboyer
Copy link
Member Author

rboyer commented Sep 11, 2015

Do I have to do anything else to help move this along?

@armon
Copy link
Member

armon commented Sep 11, 2015

@rboyer Sorry about the delay. Could you please add a test case?

@rboyer
Copy link
Member Author

rboyer commented Sep 14, 2015

Rebased and added tests.

@rboyer
Copy link
Member Author

rboyer commented Sep 22, 2015

Does this look sufficient now?

@slackpad slackpad self-assigned this Sep 24, 2015
@slackpad
Copy link
Contributor

Sorry been swamped but I'll try to get to this in the next day or so.

slackpad added a commit that referenced this pull request Sep 25, 2015
Fix Session.Renew to care about 404s properly
@slackpad slackpad merged commit c74355f into hashicorp:master Sep 25, 2015
@slackpad
Copy link
Contributor

LGTM - thanks!

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.

3 participants