-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Modify Handler.finished API to remove assumption all responses are one-shot. #81
base: main
Are you sure you want to change the base?
Conversation
@redvers I see no problem with finished getting called multiple times on the user supplied handler so long as "send finished" is only called once. Also note, that all See: be send_finished(request_id: RequestID) =>
"""
### Verbose API
Indicate that the response for `request_id` has been completed,
that is, its status, headers and body have been sent.
This will clean up resources on the session and
might send pending pipelined responses in addition to this response.
If this behaviour isnt called, the server might misbehave, especially
with clients doing HTTP pipelining.
"""
None on The update to those docs you posted would be that |
Hi @redvers, The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do. Release notes are added by creating a uniquely named file in the The basic format of the release notes (using markdown) should be:
Thanks. |
examples/hello_world/main.pony
Outdated
@@ -103,5 +103,5 @@ class BackendHandler is Handler | |||
_session.send_raw(_response, request_id) | |||
_session.send_finished(request_id) | |||
|
|||
fun ref finished(request_id: RequestID) => None | |||
// fun ref finished(request_id: RequestID): Bool => true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the commented out?
This is the part of the documentation that made me believe that Handlers don't need to deal with multiple requests at the same time:
Specifically:
If Handlers do need to be able to handle more than one request at a time then we likely need to clarify the documentation and modify the examples to do this. |
NOTE: This is a breaking change.
TODO:
✅ Update Docs
✅ Release Notes
✅ Clearer Example Maybe?
Caveat:
This may be a dragon. From handler.pony:
This raises the very real possibility that a new apply() request may come in on the same
Handler
while we're still doing our recursive calls to finished().In other words, "it is safe to keep state for the given request in the Handler between calls" may no longer be true.
More investigation required.