Thread-local keys are easy to collide with #117

Closed
halorgium opened this Issue Nov 11, 2012 · 6 comments

Comments

Projects
None yet
2 participants
@halorgium
Member

halorgium commented Nov 11, 2012

InternalPool [1] uses thread[:queue] and assumes that no-one else will use this.
TaskFiber and TaskThread use :actor, :task and :mailbox.

I recommend changing these to something including celluloid.

@halorgium

This comment has been minimized.

Show comment
Hide comment
@halorgium

halorgium Nov 11, 2012

Member

Note: this only occurs with TaskThread.

require 'celluloid'

class ClearQueue
  include Celluloid
  task_class TaskThread

  def break
    Thread.current[:queue] = nil
  end
end

cq = ClearQueue.new
cq.break
cq.break
Member

halorgium commented Nov 11, 2012

Note: this only occurs with TaskThread.

require 'celluloid'

class ClearQueue
  include Celluloid
  task_class TaskThread

  def break
    Thread.current[:queue] = nil
  end
end

cq = ClearQueue.new
cq.break
cq.break
@tarcieri

This comment has been minimized.

Show comment
Hide comment
@tarcieri

tarcieri Nov 13, 2012

Member

Good point, at the very least these should probably be :celluloid_actor, :celluloid_queue, etc.

For actors themselves I could probably subclass Thread and add the relevant fields as attributes of the current thread, so actors could be built on Celluloid::Threads and let you do Thread.current.actor without all of the logic to determine whether or not you're in a thread context like the current implementation uses.

That would give a lot cleaner API, IMO

Member

tarcieri commented Nov 13, 2012

Good point, at the very least these should probably be :celluloid_actor, :celluloid_queue, etc.

For actors themselves I could probably subclass Thread and add the relevant fields as attributes of the current thread, so actors could be built on Celluloid::Threads and let you do Thread.current.actor without all of the logic to determine whether or not you're in a thread context like the current implementation uses.

That would give a lot cleaner API, IMO

@tarcieri tarcieri closed this in 2db76eb Dec 11, 2012

tarcieri added a commit that referenced this issue Dec 11, 2012

Revert "Namespace thread locals (fixes #117)"
This reverts commit 2db76eb.

This is incompatible with Celluloid::IO, etc

@halorgium halorgium reopened this Dec 11, 2012

@halorgium

This comment has been minimized.

Show comment
Hide comment
@halorgium

halorgium Dec 11, 2012

Member

Would it make sense to add the subclass impl so that the old Thread-local keys could be maintained for a version or two?

Member

halorgium commented Dec 11, 2012

Would it make sense to add the subclass impl so that the old Thread-local keys could be maintained for a version or two?

@tarcieri

This comment has been minimized.

Show comment
Hide comment
@tarcieri

tarcieri Dec 11, 2012

Member

Nah, I think I'll just save this for 0.13.x when I can coordinate the change across all projects at the same time

Member

tarcieri commented Dec 11, 2012

Nah, I think I'll just save this for 0.13.x when I can coordinate the change across all projects at the same time

@halorgium

This comment has been minimized.

Show comment
Hide comment
@halorgium

halorgium Dec 11, 2012

Member

Rgr.

Member

halorgium commented Dec 11, 2012

Rgr.

@tarcieri

This comment has been minimized.

Show comment
Hide comment
@tarcieri

tarcieri Dec 13, 2012

Member

This is fixed in master, although ideally I think I'd like to use the "new" thread local API from Ruby 2.0. A backport should be available in the thread_safe gem soon.

Member

tarcieri commented Dec 13, 2012

This is fixed in master, although ideally I think I'd like to use the "new" thread local API from Ruby 2.0. A backport should be available in the thread_safe gem soon.

@tarcieri tarcieri closed this Dec 13, 2012

shadoi pushed a commit to shadoi/celluloid that referenced this issue Apr 18, 2013

Namespace thread locals (fixes #117)
This namespaces the four thread local names that Celluloid used
previously:

* :actor
* :mailbox
* :task
* :queue

...with celluloid_ on the front so as to avoid collisions:

* :celluloid_actor
* :celluloid_mailbox
* :celluloid_task
* :celluloid_queue

shadoi pushed a commit to shadoi/celluloid that referenced this issue Apr 18, 2013

Revert "Namespace thread locals (fixes #117)"
This reverts commit 2db76eb.

This is incompatible with Celluloid::IO, etc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment