netsuite
netsuite copied to clipboard
Copy configuration attributes from main thread to child threads
This is a follow up to #549 to improve usage in a single tenant scenario where you might configure NetSuite on the main thread (ie. via Rails initializer), then make requests in another thread (ie. Sidekiq jobs). Instead of each thread starting with an empty configuration, child threads now start with the main thread's configuration.
I'm mainly opening this just to start discussion around some concrete code. It may not be ready for a merge yet.
I'm not sure if we need an explicit single vs. multi-tenant configuration option. Having the new threads start with a copy of the main thread's configuration seems better. In a multi-tenant scenario, this way you can still set some general configuration (ie. timeouts, logging, proxy, etc.) on the main thread and have it apply to all child threads, then in the child threads you just have to configure whatever's unique to that tenant (ie. account, credentials, etc.), which wont leak back to the main thread or future child threads.
I didn't touch the caches (WSDL or record) that were changed in #549. As a result, in a single-tenant scenario with requests happening in child threads (ie. Sidekiq jobs), I think each thread (job) will cache the WSDL separately, which defeats the purpose of a cache a bit. Previously I believe the WSDL was cached globally (to the detriment of multi-tenant scenarios). Perhaps this is where the single vs multi-tenant configuration option comes into play to be explicit about where we cache? The same likely applies for the record cache. Maybe we need slightly different rules for where we store things (Thread.current vs Thread.main) whether we're talking about configuration attributes or caches.
@andrewdicken-stripe perhaps you could weigh in as the original author, assuming your use-case might be multi-tenant?
Not sure why so many CI checks were cancelled, but the few that failed I believe are due to a change in the recently released Savon 2.13, but I haven't had a change to dig into that.
This may be why we're seeing failures with savon v2.13.0: https://github.com/savonrb/savon/pull/943#issuecomment-1213163418
Only issue with this approach is it requires the NS config to be:
- Set on the main thread
- Not change in a subthread, otherwise the changes would not propagate to other threads
I think an explicit multi_threaded_configuration! which uses thread-local vars, and uses class variables otherwise is the most simple and will avoid unintended consequences.
Gotcha, I think I see what you're going for better now. Took a stab at that in #556.
We can close this out because of #556, right?