Increase maxListeners on DeltaManagerSummarizerProxy and AgentScheduler. #16216
Conversation
github-actions
bot
added
area: framework
⯅ @fluid-example/bundle-size-tests: +3.58 KB
Baseline commit: 65c8073 |
|
I don't have a particular feeling about the maximum, but it's surprising to me that you're registering listeners directly on the deltaManager - can you elaborate on what you're listening for there? |
@ChumpChief - I went ahead and messaged you privately about this. |
|
@vladsud - as mentioned above I don't have a strong opinion on the max listeners, but you might have opinions on how folks should observe the "readonly" event from DeltaManager? I was somewhat surprised to see the reach to the delta manager, wondering if there's something more appropriate to observe? |
|
I do not know much about scenario to comment, to be honest. |
Thanks for taking a look! If I understand correctly, the idea of raising "readonly" events on data stores would be a good future improvement for FF, but our current pattern (listening directly to DeltaManager) is the best way of doing so at the moment. @ChumpChief - in that case, would it be okay to approve this PR? |
|
I'll defer to Vlad since he owns readonly state - Vlad, are you saying that preferred approach is what should be done instead, or as some followup? |
|
@vladsud - I just returned from vacation, and I wanted to see if you could take a look at @ChumpChief 's question:
|
There was a problem hiding this comment.
Had a side conversation with @vladsud, he is OK with continuing this pattern for now.
This is a follow-up PR to #16216 . In testing the fix with teams, I discovered one more noisy event listener warning that was due to registrations on the `Audience` class. In the context of a table component, we would register one "addMember" event listener per cell which quickly exceeds the default "warn" limit of 10 with a reasonable-sized table. The warnings look like this: https://github.com/microsoft/FluidFramework/assets/1000369/411039ac-2c9c-4c0d-b567-26f39b2e9a9c Because these errors mention "Memory Leak" they have been a source of confusion with our partners. Ironically, until we patched our console object to stringify all error objects, the warning message itself being logged to the console was a source of memory leaks. This change removes the maxSafeListener check on the `Audience` class.
Description
Currently, when you load a Loop table component that is built with FluidFramework, you'll see these garbage errors in the console:
Because these errors mention "Memory Leak" they have been a source of confusion with our partners. Ironically, until we patched our console object to stringify all error objects, the warning message itself being logged to the console was a source of memory leaks.
This change removes the maxSafeListener check on the two classes in FluidFramework that were responsible for this noise.
The number of listeners on these classes will scale with the number of cells in your table, which could be immense. Since the "limit" is effectively
Nit seemed like removing the warnings was appropriate. They do very little to protect against actual memory leaks, which almost always come from failing to un-register a listener, not from adding too many.