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

introduce request finalizers into handler chain #9194

Merged
merged 3 commits into from Dec 19, 2023
Merged

Conversation

thrau
Copy link
Member

@thrau thrau commented Sep 21, 2023

Motivation

In the persistence plugin (and perhaps elsewhere), we are using a pattern where we are acquiring locks in request handlers, and freeing them in response handlers, to lock certain things across a request invocation. This can lead to locks never being freed, when handlers (like the CORS enforcer) call chain.terminate(), which prevents response handlers from being called.

This PR introduces the concept of finalizers into the handler chain, which are always called, even if the chain is terminated. Obviously these should be used sparsely, and only run essential finalization code.

Changes

  • Introduce the finalizer: List[Handler] attribute
  • Re-formatted a bunch of docs to match the column limit of our black config

@thrau thrau added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Sep 21, 2023
@thrau thrau added this to the Playground milestone Sep 22, 2023
@thrau thrau modified the milestones: Playground, 3.1 Dec 19, 2023
@thrau thrau requested a review from bentsku December 19, 2023 15:16
@thrau thrau marked this pull request as ready for review December 19, 2023 15:16
Copy link

S3 Image Test Results (AMD64 / ARM64)

    2 files      2 suites   3m 18s ⏱️
381 tests 331 ✔️   50 💤 0
762 runs  662 ✔️ 100 💤 0

Results for commit f9451e6.

Copy link

LocalStack Community integration with Pro

       2 files         2 suites   1h 11m 44s ⏱️
2 410 tests 2 182 ✔️ 228 💤 0
2 411 runs  2 182 ✔️ 229 💤 0

Results for commit f9451e6.

@coveralls
Copy link

Coverage Status

Changes unknown
when pulling f9451e6 on request-finalizers
into ** on master**.

Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! This is nice! Lucky for us for now, we didn't run into that kind of issues yet, but it's great to have this. Great catch! Only one small question regarding the boolean finalized, but otherwise all good!


self.stopped = False
self.terminated = False
self.finalized = False
Copy link
Contributor

Choose a reason for hiding this comment

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

q: when is this set to True? or how can you set it to True?

Copy link
Member Author

Choose a reason for hiding this comment

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

A handler could set it. It's just a way to control things for tests oder other systems that may not want/need familzers

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right yes thanks! The Handler has access to the HandlerChain. Thanks for answering 😄

@thrau thrau merged commit 16add42 into master Dec 19, 2023
33 checks passed
@thrau thrau deleted the request-finalizers branch December 19, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants