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

Task-local storage #85

Merged
merged 2 commits into from
Oct 18, 2016
Merged

Task-local storage #85

merged 2 commits into from
Oct 18, 2016

Conversation

njsmith
Copy link
Contributor

@njsmith njsmith commented Oct 17, 2016

Implement task-local storage for curio.

No tests yet, but everything else is there.

I added a nice example of the API in action to the tutorial -- probably that's the thing to look at first.

Open questions

To highlight the two API decisions that are most likely to be controversial / I'm most uncertain about:

Sync vs async

I made the interface totally usable from synchronous code. Aside from being weird, this does force certain decisions in the implementation -- in particular it requires some hoop-jumping to set up and maintain a global (thread-local) rendesvous point for locating the current task metadata. This might have performance implications -- I haven't run benchmarks. (Specifically, doing it this way means there's a bit of extra overhead at every task switch; on the other hand, I suspect but haven't checked that it makes accessing TLS values faster than it would be if we had to trap every time.)

The rationale for this decision is that a major, probably the major use case for TLS in curio is for storing contextual metadata for logging. And logging is a thing where you kind of need to make it work, even under adverse conditions. Like if you're trying to siphon out data from stdlib logging and into a better system, then you should do that and probably rewriting every third-party library to stop using stdlib logging is not a viable option -- but stdlib logging is sync-land, so if we made this an async API we'd be stuck. Also you probably don't want to ever find yourself in the situation where you want to stick some debug statement in some random leaf function somewhere to figure out why everything is on fire... but oops that leaf function is synchronous, so you can't write await log.debug(...) without first refactoring a whole call chain.

Inheritance

I made it so that child tasks inherit the initial values of TLS data from their parents (as a simple one-time shallow snapshot). This is very unusual among TLS implementations, but for a system like curio where tasks are small, cheap, and common, then I think it makes a lot of sense. Other alternatives include: (1) not inheriting, (2) fancier inheritance, like having children keep a pointer to their parent and if a key isn't found in a child traverse the graph looking for it (so changes in parents propagate to children but not vice-versa).

This does add some overhead to task spawning, though I suspect that it's very small in the common case (though it scales with the number of TLS keys in use).

@njsmith
Copy link
Contributor Author

njsmith commented Oct 17, 2016

Added tests, but ATM they just hang due to #86, so I don't know if they're correct.

@dabeaz
Copy link
Owner

dabeaz commented Oct 17, 2016

Think I managed to introduce some conflicts with recent changes. Inclined to merge though.

Implement task-local storage for curio.

No tests yet.
@njsmith njsmith force-pushed the task-local-storage branch 2 times, most recently from d28db8d to 831d580 Compare October 18, 2016 01:06
@njsmith njsmith changed the title [WIP] Task-local storage Task-local storage Oct 18, 2016
@njsmith
Copy link
Contributor Author

njsmith commented Oct 18, 2016

Rebased on master and fixed the tests. We'll see what travis says, but they pass locally now for me. (Except for tests/test_file.py::test_readiter, but that also fails for me on master -- I guess I should file a bug on that too huh :-).)

Anyway, I think it's ready for review/merge.

@dabeaz
Copy link
Owner

dabeaz commented Oct 18, 2016

I think the test_readiter test requires Python 3.5.2 or newer--there was some change in how asynchronous iteration was supported between 3.5.1 and 3.5.2. (Suppose I ought to add a version check in the test).

@njsmith
Copy link
Contributor Author

njsmith commented Oct 18, 2016

Oh right, the thing about whether __aiter__ is async or not. Yeah, that would explain it. It's not terribly hard to support both IIRC, but certainly annoying.

The travis failure is in the doc build:

/home/travis/build/dabeaz/curio/docs/reference.rst:204: WARNING: py:exc reference target not found: curio.TimeoutCancelledError

I don't think this is my fault :-)

@dabeaz dabeaz merged commit 564d2c9 into dabeaz:master Oct 18, 2016
@dabeaz
Copy link
Owner

dabeaz commented Oct 18, 2016

@njsmith
Copy link
Contributor Author

njsmith commented Oct 18, 2016

I hadn't! That is super interesting, thanks!

The first thing I learned is that apparently my shallow-copy-on-spawn idea is the same one that C# went with some years ago, so I guess that's a vote of confidence.

@@ -11,6 +11,7 @@
from .workers import *
from .network import *
from .file import *
from .tls import *
Copy link

Choose a reason for hiding this comment

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

In web context TLS usually means Transport Layer Security. Having both curio.ssl and curio.tls, where curio.ssl implements both SSL and TLS, and curio.tls has nothing to do with SSL/TLS is confusing :)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for this. I was also a bit surprised by this. This should not be a issue for users, as they wan't interact with this module directly, but this will definitely confuse someone who is willing to explore the internals of curio.

@njsmith
Copy link
Contributor Author

njsmith commented Nov 7, 2016

Yeah, I was talking about this with @1st1 last night... Maybe we should
rename to "context-local storage"

On Nov 7, 2016 3:10 AM, "Alexander Zhukov" notifications@github.com wrote:

@ZhukovAlexander commented on this pull request.

In curio/init.py #85:

@@ -11,6 +11,7 @@
from .workers import *
from .network import *
from .file import *
+from .tls import *

+1 for this. I was also a bit surprised by this. This should not be a
issue for users, as they wan't interact with this module directly, but this
will definitely confuse someone who is willing to explore the internals of
curio.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#85, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAlOaJLVPem7hPi824-EnoiVk7f8BRAzks5q7wcXgaJpZM4KYRFV
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants