Deadlock prevention - #381
Conversation
|
r? @arthurnn (@rails-bot has picked a reviewer for you, use r? to override) |
| return if gzip.cannot_compress?(environment.mime_types) | ||
|
|
||
| if File.exist?("#{target}.gz") | ||
| logger.debug "Skipping #{target}.gz, already exists" |
There was a problem hiding this comment.
There's no mutex around the logging in sprockets so logging from within a thread can result in garbled output. That's why originally it was logged outside of the pool.
There was a problem hiding this comment.
Thanks for letting me know. I'll fix it later today.
| logger.debug "Skipping #{target}, already exists" | ||
| compressor.execute if compressor | ||
| else | ||
| logger.info "Writing #{target}" |
There was a problem hiding this comment.
I think we need to run create_compressor after each time we log since it outputs to logs right away Otherwise since create_compressor immediately outputs a log, you would could get something in your output like
Writing #{target}.gz
Writing #{target}
or
"Skipping #{target}.gz, already exists"
"Skipping #{target}, already exists"
Which is backwards.
…a chain of Promises. For more information: #376
I started with #372 and made some style and behavior changes. The idea remains the same, that all writing to disk will happen via an exporter which gives us a place we can modify behavior from a gem. The interface for an exporter is as follows: An exporter is a class, a new instance will be initialized for each asset. It has access to: - asset - environment - directory - target There is a `setup` method that can be used to create mutable state before the exporter is called. A `skip?` method is run synchronously to determine if the exporter should execute or not. Since this is the only method guaranteed to run synchronously and since our logger is not threadsafe, we can do notifications in this method. The logger is passed into `skip?` to prevent people from accidentally storing a reference and trying to log when things are being called in different threads. Finally a `call` method does the work of writing a file to disk. It takes no arguments. Other changes? The top level lib/sprockets dir is really large so I moved the built in exporters to their own directory. Added documentation. Only allow registering of classes for exporters instead of procs. Added a zopfli exporter by default if you have the gem loaded before sprockets gets required. Switch back to using `compile` for the manifest interface. Use promises to chain asset compiles, this also handles #381. Fix tests to not use `rm -rf` with a system call in a few places
|
It doesn't look like calling |
|
That sounds a little different from what we're doing here. In this code, we are not waiting for the chain of promises to finish up, we're calling |
|
Sorry for stringing you along for so long. I needed to re-write some manifest logic that overlapped here and re-wrote this section. Thanks for this patch and for the original idea of limiting the threads for promises. I appreciate all your help even if this patch isn't being merged. |
|
@schneems Thank you for all the feedback, let me know if you guys need anything. |
4 participants
For more information: #376
cc @schneems @matthewd