search

causal-agent / scraper

HTML parsing and querying with CSS selectors
https://docs.rs/scraper
ISC License
1.79k stars 98 forks source link

Stop re-using NthIndexCache to avoid ABA problem #136

Closed adamreichold closed 1 year ago

adamreichold commented 1 year ago

I am really really sorry for asking for a release, but I fear we already need version 0.17.1 because I made a mistake when adding the thread-local NthIndexCache:

I just noticed that the NthIndexCache is internally keyed on the pointer to the selectors themselves which I think opens this up for an ABA problem, i.e. after dropping a selector and allocating a new one at the same address but with different content, the cache might result in incorrect matches.

Hence, I just avoided re-use completely which is certainly correct if unnecessarily inefficient, but I fear proper re-use would need to be user-visible.

adamreichold commented 1 year ago

Let's make this a draft and store the cache within the Selector struct. This should probably tie its lifecycle to that of the selector while still giving a chance of reusing e.g. during iteration and avoids the overhead of accessing TLS. It just means I need to manually implement some boiler plate traits...

adamreichold commented 1 year ago

Ok, so avoiding TLS is not possible without messing up the API because nobody gets mutable access to Selector, but re-use is still possible by tying the lifecycle to that of the selector using the thread_local crate, thereby avoiding the ABA problem while still enabling the cache to be effective.

I added this as a separate commit though, so that it can be easily dropped if we decide to go for the obviously correct if less efficient variant instead.

adamreichold commented 1 year ago

Aaaaand back again because it is not just the addresses of the selectors, but the cache also uses selectors::OpaqueElement which is basically the address of an element which means the same ABA problem if the same selector is used with multiple documents allocated at the same address.

Let's just drop the complexity and use the obviously correct approach. Sorry for the mess and the back and forth...

(I guess this might even be faster if no nth-index selectors are used because the TLS goes away.)

cfvescovo commented 1 year ago

I am really really sorry for asking for a release, but I fear we already need version 0.17.1 because I made a mistake when adding the thread-local NthIndexCache

It's my fault too, I should have caught that in my review this morning Anyway, don't worry, I will release an hotfix ASAP

Let's just drop the complexity and use the obviously correct approach.

Agreed.