-
Notifications
You must be signed in to change notification settings - Fork 172
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
Server: Worker refactor #704
Merged
Merged
Conversation
This file contains 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
tasn
reviewed
Nov 15, 2022
tasn
reviewed
Nov 15, 2022
jaymell
force-pushed
the
worker-refactor-2
branch
2 times, most recently
from
November 18, 2022 00:10
f56534d
to
dfa027c
Compare
tasn
reviewed
Nov 18, 2022
jaymell
force-pushed
the
worker-refactor-2
branch
from
November 18, 2022 05:20
7556fb3
to
caffa80
Compare
jaymell
force-pushed
the
worker-refactor-2
branch
from
December 6, 2022 16:35
caffa80
to
d1114b0
Compare
jaymell
force-pushed
the
worker-refactor-2
branch
from
January 12, 2023 23:00
d1114b0
to
0b7d764
Compare
jaymell
commented
Jan 12, 2023
tasn
reviewed
Jan 13, 2023
jaymell
force-pushed
the
worker-refactor-2
branch
5 times, most recently
from
January 13, 2023 23:21
b9c597b
to
b7c7f4e
Compare
* Add webhook-http-client, which is based on hyper, enabling several critical improvements: * Default filtering of endpoints that resolve to private address space, with ability to add CIDR exceptions as needed * Case-sensitive HTTP1 headers -- although HTTP spec states that headers are not case-sensitive, in practice many widely deployed web-servers expect to support case-sensitive headers. This requires a fork of hyper, which currently has but does not expose support for case-sensitive headers. * Assurance that the `host` header appears first in outgoing HTTP requests. Although HTTP spec does not prescribe an order for headers, in our experience, many web-servers will fail if the `host` header does not appear first in request headers. We ensure this by manually adding the header ourselves rather than relying on hyper to do so. * Use `openssl` instead of `rusttls` for outbound webhooks. In our experience, many widely deployed webservers use "weak" ciphers that rusttls refuses to support, leaving us with no choice but to rely on openssl libraries. * Support for soft-limiting of concurrent worker tasks. Unbounded concurrent worker tasks can easily overwhelm a system. This sets a default of 500 tasks with support for configuring other limits or setting unlimited (`0`) concurrent tasks * Refactor message dispatch code for clarity. Dispatch code is now broken into separate methods: `prepare_dispatch`, `make_http_call`, `handle_successful_dispatch`, and `handle_failed_dispatch` with the dispatch itself having different types depending on its stage in the dispatch process. Other function names have also been changed for improved clarity. * Prevent worker from shutting down when there are active tasks. * Avoid multiple DB calls for message-destination insert/retrieval
jaymell
force-pushed
the
worker-refactor-2
branch
from
January 17, 2023 16:07
b7c7f4e
to
5ed220e
Compare
jaymell
commented
Jan 17, 2023
jaymell
commented
Jan 17, 2023
jaymell
commented
Jan 17, 2023
jaymell
commented
Jan 17, 2023
jaymell
commented
Jan 17, 2023
jaymell
commented
Jan 17, 2023
jaymell
commented
Jan 17, 2023
jaymell
commented
Jan 17, 2023
tasn
approved these changes
Jan 17, 2023
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.
critical improvements:
address space, with ability to add CIDR exceptions as needed
headers are not case-sensitive, in practice many widely
deployed web-servers expect to support case-sensitive headers.
This requires a fork of hyper, which currently has but does
not expose support for case-sensitive headers.
host
header appears first in outgoing HTTP requests.Although HTTP spec does not prescribe an order for headers, in
our experience, many web-servers will fail if the
host
header doesnot appear first in request headers. We ensure this by manually
adding the header ourselves rather than relying on hyper to do so.
openssl
instead ofrusttls
for outbound webhooks. In our experience,many widely deployed webservers use "weak" ciphers that rusttls refuses to
support, leaving us with no choice but to rely on openssl libraries.
tasks can easily overwhelm a system. This sets a default of 500 tasks with support
for configuring other limits or setting unlimited (
0
) concurrent tasksseparate methods:
prepare_dispatch
,make_http_call
,handle_successful_dispatch
, andhandle_failed_dispatch
with the dispatch itselfhaving different types depending on its stage in the dispatch process.
Other function names have also been changed for improved clarity.