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() is un-Julian #14135

Open
stevengj opened this issue Nov 25, 2015 · 13 comments
Open

task_local_storage() is un-Julian #14135

stevengj opened this issue Nov 25, 2015 · 13 comments
Labels
multithreading Base.Threads and related functionality

Comments

@stevengj
Copy link
Member

Seems like it would be much more idiomatic to have a global variable TASK_LOCAL_STORAGE::Task_Local_Storage with get, getindex, and setindex! methods for Task_Local_Storage <: Associative{Any,Any} so that you could just do TASK_LOCAL_STORAGE[:foo] = bar etcetera.

I noticed this because there is no documented analog of get(TASK_LOCAL_STORAGE, key, default) for task_local_storage() ... I expected task_local_storage(key, default) to do this and was surprised that it acted as assignment (without !). There is the undocumented get(task_local_storage(), key, default), but it occurred to me that just having TASK_LOCAL_STORAGE act like a Dict would be much cleaner and clearer.

@stevengj stevengj added the needs decision A decision on this change is needed label Nov 25, 2015
@JeffBezanson
Copy link
Sponsor Member

This is a good idea. I only wonder if it's ever useful to get a reference to the actual storage of a specific task. Would we do that via TASK_LOCAL_STORAGE[]?

@stevengj
Copy link
Member Author

stevengj commented Dec 1, 2015

@JeffBezanson, in all the cases that I can find that currently call task_local_storage() (to fetch the Dict), it is to call methods like get and delete! that could instead be defined directly for TASK_LOCAL_STORAGE.

@IainNZ
Copy link
Member

IainNZ commented Dec 1, 2015

👍, I experienced this when I redeveloped Base.Test and it felt like very clunky

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Sep 13, 2016
@tkelman tkelman modified the milestones: 1.0, 0.6.0 Jan 5, 2017
@tkelman
Copy link
Contributor

tkelman commented Jan 5, 2017

unlikely to be refactored right now

@Keno Keno added help wanted Indicates that a maintainer wants help on an issue or pull request and removed needs decision A decision on this change is needed labels Jul 20, 2017
@Keno
Copy link
Member

Keno commented Jul 20, 2017

Discussed during triage and decided that having a special array type seems like a nice design. Removing decision, adding up for grabs.

@vtjnash vtjnash self-assigned this Sep 5, 2017
@StefanKarpinski
Copy link
Sponsor Member

Could we just express this with indexing into type objects? E.g. task[:key] and task[:key] = val.

@stevengj
Copy link
Member Author

@StefanKarpinski, you mean into the Task type? (Instead of defining a global?) That seems like a cute design, albeit a little unusual.

@StefanKarpinski
Copy link
Sponsor Member

No, I meant the task object, but the task type could work too. The task object would allow accessing a tasks storage externally.

@stevengj
Copy link
Member Author

So you would do current_task()[:foo]? That seems fine to me.

@JeffBezanson
Copy link
Sponsor Member

I'd be a little worried that that would prevent us from adding other operations on Tasks, e.g. we used to have task iteration (for x in task), which would now be iterating over the storage.

@StefanKarpinski
Copy link
Sponsor Member

It's not entirely kosher but you could distinguish indexing into a task with integers versus symbols...

@StefanKarpinski
Copy link
Sponsor Member

Another thought: have a Base.Tasks module so as to avoid polluting the Base namespace with generic names related to tasks (or use Base.Threads) and have Tasks.storage() for the task-local-storage of the current task and Tasks.storage(task) for the task-local storage of another task.

@Keno
Copy link
Member

Keno commented Nov 16, 2017

This seems non-breaking. Worst case we have to keep the old function as an alias. Propose to remove from milestone.

@Keno Keno added the triage This should be discussed on a triage call label Nov 16, 2017
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Nov 20, 2017
@StefanKarpinski StefanKarpinski modified the milestones: 1.0, 1.x Nov 20, 2017
@vtjnash vtjnash removed the help wanted Indicates that a maintainer wants help on an issue or pull request label Apr 17, 2019
@vtjnash vtjnash removed this from the 1.x milestone Apr 17, 2019
@brenhinkeller brenhinkeller added the multithreading Base.Threads and related functionality label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

No branches or pull requests

9 participants