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

Easy API for having context span threads #247

Merged
merged 5 commits into from
Dec 7, 2015
Merged

Conversation

itamarst
Copy link
Owner

@itamarst itamarst commented Dec 4, 2015

Fixes #225.

"""
with startAction(action_type="parent"):
wrapped = preserve_context(self.add)
self.assertEqual(wrapped(3, y=4), 7)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lines 1424 and 1425 in a helper function that this test method and test_no_context can both call?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think with only two callers and two lines it doesn't end up being shorter. But yeah, if there were more of them.

@exarkun
Copy link
Collaborator

exarkun commented Dec 5, 2015

Thanks. This is a nice addition.

Relatedly, this past week I've been noticing that even some single-threaded code manages to lose the parent/child association - if it's asynchronous. Lots of code uses Deferred without being Eliot-aware at all. Any time execution has to jump to a callback on a Deferred, the Eliot context gets lost. That's a bit sad.

It seems like this preserve_context is part of a solution for that problem. It doesn't have to be used across threads, after all. Though maybe "remote_task" isn't the best way to represent "asynchronous mumble mumble" (it's not how DeferredContext glues things together, after all). But it doesn't help with the "this code uses Deferred and doesn't know about Eliot problem" though ... so ... I dunno.

I guess thread-using code may possibly suffer an equivalent problem. This helper doesn't help if you don't use it and you may want to call existing code that doesn't know about Eliot. Not sure what to do about that, though.

Anyhow, this looks like a useful helper for the case where Eliot-aware code does control the dispatch-to-thread piece, so, great.

Just a few minor points inline. Address to your satisfaction and then merge.

@itamarst
Copy link
Owner Author

itamarst commented Dec 7, 2015

Yeah, making stuff Just Work is hard with async and threaded code. A sub-class of Deferred might work in some cases, but sooner or later you'd hit a Deferred someone else created.

itamarst added a commit that referenced this pull request Dec 7, 2015
Easy API for having context span threads.

Fixes #225.
@itamarst itamarst merged commit 88698c7 into master Dec 7, 2015
@itamarst itamarst deleted the thread-context-225 branch December 7, 2015 16:37
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.

2 participants