Refactor Observer and LimitEnforcer cleaner for... #3114
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
danlapid
force-pushed
the
dlapid/refactor_limitenforcer_workerobserver
branch
2 times, most recently
from
8a34223 to
b026443
Compare
danlapid
commented
Nov 14, 2024
danlapid
commented
Nov 14, 2024
danlapid
commented
Nov 14, 2024
danlapid
force-pushed
the
dlapid/refactor_limitenforcer_workerobserver
branch
2 times, most recently
from
9349b16 to
f5bc217
Compare
|
More info on internal PR #9192 |
danlapid
changed the title
|
Much of this is basically a revert of #3034 for an even cleaner implementation. |
danlapid
force-pushed
the
dlapid/refactor_limitenforcer_workerobserver
branch
from
f5bc217 to
d7ad08e
Compare
jasnell
reviewed
Nov 20, 2024
| @@ -305,11 +307,11 @@ class Worker::Isolate: public kj::AtomicRefcounted { | |||
| kj::Maybe<ValidationErrorReporter&> errorReporter = kj::none) const; | |||
|
|
|||
| inline IsolateLimitEnforcer& getLimitEnforcer() { | |||
| return limitEnforcer; | |||
| return *limitEnforcer; | |||
| } | |||
|
|
|||
| inline const IsolateLimitEnforcer& getLimitEnforcer() const { | |||
There was a problem hiding this comment.
Non-blocking nit: We have the kj::Rc<T> and kj::Arc<T> types now for safer handling of references to refcounted and atomic refcounted types. It would be nice if we could start working on replacing uses of bare references for these where it's not too cumbersome of a change.
jasnell
approved these changes
Nov 20, 2024
danlapid
force-pushed
the
dlapid/refactor_limitenforcer_workerobserver
branch
from
d7ad08e to
a35812d
Compare
danlapid
force-pushed
the
dlapid/refactor_limitenforcer_workerobserver
branch
2 times, most recently
from
f3d3fb4 to
91c0568
Compare
jp4a50
approved these changes
Dec 5, 2024
danlapid
force-pushed
the
dlapid/refactor_limitenforcer_workerobserver
branch
2 times, most recently
from
c9a3d5c to
9a5a6ee
Compare
This will return ownership over limitEnforcer from the api class to the isolate class.
This will allow for better separation of responsibilities between the two.
danlapid
force-pushed
the
dlapid/refactor_limitenforcer_workerobserver
branch
from
9a5a6ee to
550e085
Compare
danlapid
added a commit
that referenced
this pull request
Dec 7, 2024
…r_workerobserver Refactor Observer and LimitEnforcer cleaner for...
danlapid
added a commit
that referenced
this pull request
Dec 7, 2024
…r_workerobserver Refactor Observer and LimitEnforcer cleaner for...
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.
isolate pools