Java exception escaping Hash#[]= in addressable
#3166
Comments
|
This looks like concurrent mutation of a It's often worthwhile to fix in the gem because it can fix related concurrency issues. We also plan to make Hash thread-safe in general, like CRuby. |
|
https://github.com/sporkmonger/addressable/blob/addressable-2.8.4/lib/addressable/uri.rb#L354 A simple solution would be to use a Concurrent::Map from concurrent-ruby instead of raw |
|
Upstream issue: sporkmonger/addressable#514 |
Who is doing that, not addressable, right?
Would that solve sporkmonger/addressable#514? |
I guess it can be hard to say where this ought to be fixed. If it's done in addressable, it can be fixed in one spot. If not done in addressable, each caller would be responsible for wrapping addressable calls in a I'm in favor of addressing it in addressable because it can be done far more granularly. Only the calls that need special attention can be updated, whereas delegating to a client means no call can be deemed safe without docs explicitly saying so. As for whether to rely on concurrent-ruby, I have less of an opinion. It certainly would address the problem and likely in the most performant way, but it is another dependency. |
|
Sorry, I didn't fully realise what was going on. This should be addressed (hehe) in addressable. Too bad CRuby didn't want to address this, seems like it will continue to bite people running non-CRuby... |
|
If I may, I think the problem here is beyond the unsynchronized access. How many possible entries is there in these hashes? Either they are small enough and should be generated upfront, or either they are too big, and they shouldn't be lazily generated like this as it's pretty much a memory leak. (sorry it's a quick drive by comment, I haven't fully dug in the implementation yet). |
|
@byroot the hashes was introduced by sporkmonger/addressable#329 as caches to decrease object allocations, so yes, sounds to me that they could possibly grow indefinitely. Should that change be reverted instead? |
|
IMHO yes. Now I understand that doing string escaping in Ruby can be quite costly, so it's a bit of a balancing act. But to me such an unbounded global cache is very smelly. I'm a bit busy today, but I'll try to find some time to see if we can optimize this escaping enough that the cache wouldn't be necessary. Or alternatively, a precomputed, fixed sized hash could be used for the common values (e.g. common ASCII characters) and anything outside of that could not be cached. NB: All this is a drive by comment, I didn't have time to dig much into the implementation yet. |
|
(I just realized I'm commenting on Truffle instead of the issues in Addressable, sorry I haven't finished my coffee). |
eregon
changed the title
Hash#[]=Hash#[]= in addressable
Fix: sporkmonger#514 Fix: oracle/truffleruby#3166 These hashes lazily memoize percent encoded characters, this is an issue on GVL-less Ruby implementations as it can cause concurrent access. But these are actually quite wastful as the key is always a single byte so rather than use string keys are lazily memoize these, we can precompute two static arrays of 255 elements once and for all.
Fix: sporkmonger#514 Fix: oracle/truffleruby#3166 These hashes lazily memoize percent encoded characters, this is an issue on GVL-less Ruby implementations as it can cause concurrent access. But these are actually quite wastful as the key is always a single byte so rather than use string keys are lazily memoize these, we can precompute two static arrays of 255 elements once and for all.
Fix: sporkmonger#514 Fix: oracle/truffleruby#3166 These hashes lazily memoize percent encoded characters, this is an issue on GVL-less Ruby implementations as it can cause concurrent access. But these are actually quite wastful as the key is always a single byte so rather than use string keys are lazily memoize these, we can precompute two static arrays of 255 elements once and for all.
Fix: sporkmonger#514 Fix: oracle/truffleruby#3166 These hashes lazily memoize percent encoded characters, this is an issue on GVL-less Ruby implementations as it can cause concurrent access. But these are actually quite wastful as the key is always a single byte so rather than use string keys are lazily memoize these, we can precompute two static arrays of 255 elements once and for all.
Fix: #514 Fix: oracle/truffleruby#3166 These hashes lazily memoize percent encoded characters, this is an issue on GVL-less Ruby implementations as it can cause concurrent access. But these are actually quite wasteful as the key is always a single byte so rather than use string keys are lazily memoize these, we can precompute two static arrays of 255 elements once and for all. Co-authored-by: Jean Boussier <[email protected]>
|
Fixed in sporkmonger/addressable#515 by @byroot |
Released with Addressable 2.8.5 |
I've run into a case where a Java exception is escaping
Hash#[]=during our application deployment phase. It occurred while running a Rake task to upload precompiled assets to a cloud storage location. Unfortunately, the gem in question is closed source so I can't share the code that induced it. Moreover, I've only seen the failure once so I can't easily reproduce it. We may have to debug by inspection.The text was updated successfully, but these errors were encountered: