Cleared all interpreter warnings. #385
Conversation
jdantonio
commented
Jul 29, 2015
jdantonio
commented
- Added parenthesis when using splat operator
- Used _ for unused variables
- Renamed variables when block echoed surrounding scope
- Configuration now autoloads lazily.
- Cleared all circular references.
| autoload :Delay, 'concurrent/delay' | ||
| autoload :ProcessorCounter, 'concurrent/utility/processor_counter' | ||
| autoload :TimerSet, 'concurrent/executor/timer_set' | ||
| autoload :ImmediateExecutor, 'concurrent/executor/immediate_executor' |
There was a problem hiding this comment.
Why the switch to autoload? I remember a post from Matz about it having thread safety issues. Is that still true?
There was a problem hiding this comment.
That's the question at hand. None of the changes in this PR are necessary, just a good opportunity for discussion.
The issue is that we create a lot of circular references to concurrent/configuration. So far this hasn't been a problem but it's a source of concern. During my #386 experimentation I noticed that thread_safe uses autoload so I thought I'd give it a try. All the tests pass and the interpreter warnings have gone away.
There was a problem hiding this comment.
I would rather shuffle the file content to avoid the circularity if possible than using autoload. Not sure if it can be easily done though :/
There was a problem hiding this comment.
According to this it seems that autoload is now thread-safe. I've reached out to @headius and @tenderlove over Twitter for clarification.
There was a problem hiding this comment.
Here's the response: "autoload itself is safe but class definition happening in parallel can still expose incomplete classes."
There was a problem hiding this comment.
The big problem is Delay. We use Delay extensively to lazy-load resources--especially in configuration.rb. Delay itself is relatively uncoupled from everything else. The rabbit-hole is Executor.executor_from_options(opts). Many of our abstractions, including Delay, use that function to initialize an executor. That function, in turn, uses the global executor declarations from configuration.rb, most of which are lazy-loaded with Delay.
Since Delay doesn't need configuration data until an object is constructed we could potentially lazy-load configuration w/i Delay and possibly avoid the circular dependency.
There was a problem hiding this comment.
We could also clear this up by creating a boot (or core) file which will always load the minimal required files for the concurrent-ruby to function it would load these problematic files. All the other abstractions would load boot first and then any other required abstractions. This way the circular dep. would be always handled by boot.
| def pr_async_callback_on_completion(state, executor, callback) | ||
| pr_with_async(executor, state, callback) do |state, callback| | ||
| def pr_async_callback_on_completion(_state, _executor, _callback) | ||
| pr_with_async(_executor, _state, _callback) do |state, callback| |
jdantonio
force-pushed
the
warnings
branch
2 times, most recently
from
df197d6 to
cb52a93
Compare
|
This update gets rid of the autoload from configuration. The only remaining warnings are the circular references. Since there have not been a problem thus far I don't think it's a critical thing to change right now. I like the idea from @pitr-ch of having a "boot" file. That's something that can probably wait until the 2.0 release. |
jdantonio
force-pushed
the
warnings
branch
7 times, most recently
from
7d4a4cb to
1e50c57
Compare
* Added parenthesis when using splat operator * Used _ for unused variables * Renamed variables when block echoed surrounding scope * Configuration now autoloads lazily. * Cleared all circular references.
Cleared all interpreter warnings.
