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.
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
IOLocal
propagation for unsafe access #3636IOLocal
propagation for unsafe access #3636Changes from 18 commits
0b88c01
db743e2
716ef32
0a69caf
2775064
270764f
d55489d
2cf72a5
cb3859d
7dce01c
5e171ac
c2f312d
638930d
9174c6a
1987e3a
02a43a6
a7bf748
145fc0e
fa99a5c
6cad03c
bb5d4b1
7517755
8d8e004
3589db4
522677e
6cc4d38
ac88480
49e5c30
925f504
d63a6ff
d4549fb
2502045
535fc8a
d854799
f070552
2cf1d8a
0eec9dd
af84973
1adf368
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
it would be very helpful if this was exposed to the end user in some way so that they can check if it's enabled and potentially raise an error if not (for example, we don't want people trying to use an
IOLocal
-backedContextStorageProvider
and its associatedLocal
in otle4s if this value isfalse
)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.
Great point. fa99a5c, wdyt?
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.
lgtm, cheers!
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.
very cool!
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.
That's a great suggestion. I also wanted to ask something similar.
Could this be a more explicit option type like Monix's so that more options can be added when necessary?
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.
You mean so that it can be configured by user-code at runtime?
The advantage of using system properties with static final fields is that their values are constant at JVM startup, which allows the JIT to optimize those branches. Allowing dynamic configuration would circumvent that.
Or if I missed your point, sorry, please explain 😅 note we also have a lot of configuration available in
IORuntimeConfig
class which can be configured programmatically at runtime.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.
@armanbilge Oh, I didn't mean to configure it at runtime. I meant that it would be good to have a proper type for setting local propagation instead of having just
Boolean
.But, I got your point of
static final
, so please ignore what I said. 😅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.
Got it, thanks :) Monix also seems to be using just a
Boolean
for setting local propagation anyway. The difference is that it allows it to be configured dynamically per task.https://github.com/monix/monix/blob/952034ad4884b64fb4241d78f33fdef07f267157/monix-eval/shared/src/main/scala/monix/eval/Task.scala#L4406
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.
Dumb question: can't we simply do this when we get scheduled on a thread? We know when we're on a thread and we know when we get off of it, so can't we simply set and clear the state respectively at those points?
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.
No we can't, unless we unify how the state is represented. Currently it's a var to an immutable map in the fiber and also in the thread. While the fiber is running its copy of the var may be updated effectually in the runloop so the thread-local copy would need to be kept in sync with that. Or we could drive all updates through the thread-local copy of the var, but then there would be a penalty for accessing it esp. if we are not running on a worker thread.
Putting aside technical issues, nobody should be unsafely messing about with
IOLocal
s outside of a properly suspended side-effect block and this strategy enforces that.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.
What we can do is set the current fiber in a thread local every time we get scheduled on a thread. Then the unsafe
IOLocals
manipulations can operate on the state via the current fiber and we don't need to pay the penalty for everydelay
block. Based on the benchmarks this strategy is seeming more attractive 😅Note this would leave the fiber's
IOLocal
state exposed to unsafe manipulations outside ofdelay
blocks.