When RtdPublisher returns cached objects they are getting mixed up between topics

[andyvandy]

Hi Steven,

When returning a python object from an RtdPublisher without a converter, the automatic cache key generation ends up using a shared cache for all publishers publishing that object type. This results in values from different topics appearing in the wrong cells.

Details

We call FromPyObj() in publish which eventually calls ReturnToCache()

xloil/libs/xlOil_Python/PyRtd.cpp

Lines 301 to 303 in 6d4ba1b

	xlValue = converter
	? (*converter)(ptr)
	: FromPyObj()(ptr);

Since the caching isn't being called from a cell, xlfCaller fails to return an address in CallerInfo() which causes us to fall back to "Unknown" in writeAddressImpl. The value is then cached and the cache key is broadcast.

xloil/src/xlOil-XLL/Caller.cpp

Lines 396 to 401 in 6d4ba1b

	CallerInfo::CallerInfo()
	{
	callExcelRaw(xlfCaller, &_address);
	callExcelRaw(xlSheetNm, &_sheetName,
	_address.isType(ExcelType::RangeRef) ? &_address : &theA1Ref);
	}

xloil/src/xlOil-XLL/Caller.cpp

Lines 380 to 381 in 6d4ba1b

	default: // Other callers
	constexpr wchar_t nonWorksheetCaller[] = L"Unknown";

When rtdpeek or rtdsubscribe get a value like 心[nknownExample,C, they will look to the cache and retrieve a value from the cache and then replace it back into their respective cellcache but with many publishers, it's likely the value was intended for another topic.

Repro

The following formulas can be copied into and new workbook with the second row dragged down a handful of times to increase the load.

blank|=ROW(A1)&";"&COLUMN(A1) |=RtdExample(B1)|=xloAttr(C1,"value")|=COUNTA(UNIQUE(D:D))|=COUNTA(UNIQUE(B:B))
blank|=ROW(A2)&";"&COLUMN(A2) |=RtdExample(B2)|=xloAttr(C2,"value")|

import asyncio
from dataclasses import dataclass
import time
import xloil as xlo

@dataclass
class Example:
    value: str

if "_rtdServer" not in globals():
    # don't recreate on reload!
    _rtdServer = xlo.RtdServer()

class RtdPublisherExample(xlo.RtdPublisher):
    """lifted from https://xloil.readthedocs.io/en/stable/xlOil_Python/ExampleRTD.html"""
    def __init__(self, input_string:str):
        super().__init__()  # You *must* call this explicitly or the python binding library will crash
        self._python_object = Example(input_string)
        self._topic = input_string
        self._task = None
        
    def connect(self, num_subscribers):
        if self.done():
            async def run():
                try:
                    while True:
                        _rtdServer.publish(self.topic(),Example(self._topic + " " + str(time.time())))
                        await asyncio.sleep(0.01)
                        # await asyncio.sleep(1.1)
                except Exception as e:
                    _rtdServer.publish(self._url, e)
        
            self._task = xlo.get_event_loop().create_task(run())
            
    def disconnect(self, num_subscribers):
        if num_subscribers == 0:
            self.stop()
            return True # This publisher is no longer required: schedule it for destruction
            
    def stop(self):
        pass

    def done(self):
        return self._task is None or self._task.done()
        
    def topic(self):
        return self._topic


@xlo.func(local=False)
def RtdExample(input_string):
    if _rtdServer.peek(input_string) is None:
        publisher = RtdPublisherExample(input_string)
        _rtdServer.start(publisher)
    return _rtdServer.subscribe(input_string)      

Next Steps

I took a stab at addressing this by explicitly providing the topic as a cache key which resolved the collisions between topics but didn't fully resolve the issues.

PyRtd.cpp

FromPyObj < detail::ReturnToSpecifiedCache, true > (detail::ReturnToSpecifiedCache{topic})(ptr);

BasicTypes.h ( I also added a simple pyCacheAddByKey )

/// <summary>
/// Used with FromPyObj to return unknown objects as a cache ref using a specific key rather than relying on CallerInfo
/// </summary>
struct ReturnToSpecifiedCache
{
    const wchar_t* key;

    // Constructor requires key to be provided and Delete default constructor to prevent uninitialized usage
    explicit ReturnToSpecifiedCache(const wchar_t* k) : key(k) {}

    ReturnToSpecifiedCache() = delete;

    template <class TAlloc>
    auto operator()(PyObject* obj, const TAlloc& stringAllocator)
    {
        return ExcelObj(BasicPString<wchar_t, TAlloc>(pyCacheAddByKey(PyBorrow(obj),key).asStringView(), stringAllocator));
    }
};

However I see an issue with this when the excel calc frequency is less than the RTD publisher frequency. When multiple values are cached to the same cellcache we increment the cache counter.

RTD peek will fetch the cache key (eg 心[topicABC,C ) on line 328 but not the cached value. If the calc cycle has ticked forward and the cache was just cleared then there may not be enough values when we use the cache key to retrieve the python object.

xloil/libs/xlOil_Python/PyRtd.cpp

Lines 323 to 335 in 6d4ba1b

	py::object peek(const wchar_t* topic, IPyFromExcel* converter = nullptr)
	{
	shared_ptr<const ExcelObj> value;
	{
	py::gil_scoped_release releaseGil;
	value = impl().peek(topic);
	}
	if (!value)
	return py::none();
	return PySteal<>(converter
	? (*converter)(*value)
	: PyFromAny()(*value));
	}

I think we can address this by not allowing multiple separate auto cached values for RTDPublishers and to limit the cache size to 1. iterables of cacheables would need be cached directly rather than be split out.

I recall previously you mentioned you had thoughts of a grander overhaul of caching. I'd be very interested to hear your thoughts on how you'd like to approach this. In the mean time I'll attempt getting a POC PR assembled.

---

As always hope all is well and appreciate you sharing this library!

[cunnane]

There's a converter argument to the publish method, so something like this should help

to_cache = xlo._CustomReturn(lambda x: xlo.cache.add(x, tag=f"{server_id}{topic}"))
server.publish(, value, converter=to_cache)

Note the ugliness here could be encapsulated into an xloil.ReturnToCache class. However, this approach has a couple of drawbacks:

1. When the publish method is called you don't have the cell address of the cell calling subscribe. This shouldn't matter as there should be only one current value for any given topic, but it might look strange.
2. The cache objects will not be cleaned up if the server is stopped, i.e. it will leak, although once per topic rather than once per update

Alternatively, we can sidestep the cache entirely, since we only need the RTD mechanism to trigger the cell recalculation

_THE_ACTUAL_VALUES=dict()  # Would need to be made threadsafe

class RtdPublisherExample(xlo.RtdPublisher):
    ...
    def connect(self, n):
        if self._task is None:
            async def run():
                try:
                    while True:
                        _THE_ACTUAL_VALUES[self.topic()] = LargeObject(...)
                        _rtdServer.publish(self.topic(), True)  # I think it's ok to keep returning the same value
                        await asyncio.sleep(0.01)
                except Exception as e:
                    _rtdServer.publish(self.topic(), str(e))
        
            self._task = xlo.get_event_loop().create_task(run())
      ...

@xlo.func(local=False)
def RtdExample(input_string):
    start_if_required(_rtdServer, topic=input_string)
    _rtdServer.subscribe(input_string)
    return _THE_ACTUAL_VALUES[input_string]

Some of the boiler-plate here like start_if_required and the thread-safe handling of the values could be incorporated into xloil's RtdServer. Btw there's no point cycling the publish loop every 0.01s as Excel only updates RTD every 2s (this is configurable, although it's a global setting).

Disclaimer: I haven't actually checked this but something very much like this should work!

[andyvandy]

This is a really clever workaround to simply use the RTD publisher as a means to trigger the recalculation!

(un?)fortunately I managed to get a PR mostly assembled yesterday before seeing your reply. I believe it follows the principle of least astonishment and fixes this in a manner such that end users don't need to be aware of the implementation details.

#142 I've tested locally on 64bit with python 3.11 and it's worked fine but I'd be really grateful if you've got feedback on edge cases or could kick the tires. I'm happy to add some testcases if you've got some specific ones in mind.

Yeah, I've reduced the Application.RTD.ThrottleInterval to be 5-50 ms. I've found this to make certain usecases feel much more responsive. e.g. I have a filewatcher function driven by the watchdog package and it feels much snappier when the file changes flow through instantly.

PS: I messed up and included some whitespace changes so it can be easier to view with the ignore whitespace toggle

[cunnane]

I took a look at the PR and made some comments, it seems good and I think it's a sensible idea to have the caching built in rather then using my cheat mode above, but I think you can simplify the logic a bit and perhaps restrict it to the python part of the library.

With changing the ThrottleInterval, there's a small health warning: it's a global setting, so if you have any other RTD addins, they may start to snarl up your Excel. Some parts of the RTD loop have to run on the main thread along with Excel's UI and whilst xloil does its best to keep RTD logic off the main thread, other addins may not so the UI may get laggy.

[andyvandy]

Thanks for taking a look! I can't seem to see any comments though- possible they didn't go through? I can give it a shot over the next few days.

I'll keep an eye out for the ThrottleInterval but I've luckily been spared on that front so far. Thanks for the warning since I have a feeling I'll eventually need it.

[cunnane]

Ah yes, I have to click submit review...

[andyvandy]

Thanks! I'm on the road this week so I'll look on the weekend when I'm reunited with my desktop!

[andyvandy]

I took another stab in #143 . This time much more direct and simpler rather than trying to extend ObjectCache.

Do you see any glaring holes with memory cleanup/suggestions of how to tie this up?

Also this does change the behaviour of how exceptions are passed. Rather than a string, it's the exception object. This could be a breaking change so I've set the PR as a draft only.

[andyvandy]

Thanks for the diligent review of my PR! Think we could release a new version soon?

There's still the question now of how to handle the same issue with the async def defined rtd functions. The logic is a bit different from the RTDPublishers. Did you have an approach in mind or should I take it away and come up with something?

[cunnane]

Have just released 0.21, let me know if it's working. For the async def functions, the RtdReturn object holds a reference to the caller which it adds to cache string, so things should work, do you have an example where it doesn't?

[andyvandy]

Have just released 0.21, let me know if it's working. For the async def functions, the RtdReturn object holds a reference to the caller which it adds to cache string, so things should work, do you have an example where it doesn't?

Amazing!

I'll double check early next week and share a repro if it doesn't work.

[andyvandy]

Sorry I let this fall off my radar. Here's a somewhat messy example of the behaviour:

import time
import xloil as xlo 

@dataclass
class Example:
    value: str

#returns #value
@xlo.func(local=False)
async def RtdExampleAsync(input_string):
    yield Example(input_string+ " " + str(time.time()))

#returns #value
@xlo.func(local=False)
async def RtdExampleAsync3(input_string):
    return Example(input_string+ " " + str(time.time()))
 
#returns value 欣[nknown_Example,H
@xlo.func(local=False)
async def RtdExampleAsync4(input_string)-> xlo.Cache:
    return await RtdExampleAsync(input_string)