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

Work cancellation and timeout handling #113

Closed
wants to merge 1 commit into from

Conversation

kennethlakin
Copy link
Contributor

This addresses Issue #109 . See the commit comments for more details.

Do note that it's based off of my check_uri-handle-iodata branch, mentioned in PR #105 . (This is probably the source of the branch conflict notification below.) I can rebase it if you like, but the iodata handling change is small and nice to have. :)

@elbrujohalcon
Copy link
Member

@kennethlakin #105 was merged, but this one still needs rebasing

@kennethlakin
Copy link
Contributor Author

Rebased.

I don't know why Elvis is complaining here. make elvis has no complaints on my machine.

@kennethlakin
Copy link
Contributor Author

Actually, hang on. There are some whitespace changes that I accidentally un-did. Gimmy a few minutes to fix that.

@kennethlakin
Copy link
Contributor Author

Okay. Whitespace issues are fixed. Feel free to merge.

@@ -393,12 +420,14 @@ at_rest(timeout, State) ->
ok = gen_fsm:send_event(self(), Work),
{next_state, at_rest, NewState}
end;
at_rest({get_async, {HandleEvent, AsyncMode}, Args, From},
at_rest({get_async, {HandleEvent, AsyncMode},
{ReqRef, _, _, _} = Args, From},
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check this line's indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. That line's indentation now matches the gun_response clauses of wait_response/2.

@kennethlakin
Copy link
Contributor Author

Okay. The line in question's indentation now matches the gun_response clauses of wait_response/2.

This addresses Issue inaka#109, and then some.

When a request is made, shotgun:request makes a fresh reference
and passes that along with the work to the shotgun FSM.
When a request times out, shotgun:request now sends an async
request to cancel the now-useless unit of work, passing along
the reference for identification, before notifying the client
of the request timeout.

Things to know:

* Gun has its own request queue. So, if a request has made it
  to gun, cancellation of the request will only squelch future
  messages that would have come from that request. It is
  -however- safe to bounce back to at_rest (as we now do), as
  messages from the cancelled request will not be sent back to
  the FSM.
* Removal of queued requests makes use of queue:filter. For large
  queues, this is kinda not fast.

Minor changes:

* state typedef moved up with the other typedefs.
* enqueue_work_or_stop slightly simplified.
@kennethlakin
Copy link
Contributor Author

It occurred to me that I had inadvertently changed the return value when shotgun:request/6 times out from {error, {timeout, _}} to {error, timeout}. That's fixed now.

@kennethlakin
Copy link
Contributor Author

Okay, I played around with this some more this weekend and discovered a serious bug. I'll close the PR for now and reissue it when I've pushed the fix.

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