PivotGrid: — fix memory leak PivotGridDataSource(T1317109)#32403
PivotGrid: — fix memory leak PivotGridDataSource(T1317109)#32403aleksei-semikozov wants to merge 6 commits intoDevExpress:26_1from
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a memory leak in the PivotGrid component's DataController by ensuring that event handlers are properly unsubscribed from the PivotGridDataSource before disposal.
Changes:
- Fixed memory leak by always unsubscribing event handlers from data sources before disposal, regardless of whether they are shared
- Refactored event subscription logic into separate, reusable methods for better maintainability
- Optimized constructor to prevent redundant data loading when using shared PivotGridDataSource instances
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/pivot_grid/m_widget.ts | Changed inline conditional disposal to explicit if block for clarity and consistency |
| packages/devextreme/js/__internal/grids/pivot_grid/data_controller/m_data_controller.ts | Fixed memory leak by refactoring event handler management and ensuring proper cleanup in all scenarios; added conditional loading to prevent redundant load operations for shared data sources |
| _unsubscribeFromDataSource() { | ||
| const that: any = this; | ||
|
|
||
| if (!that._dataSource || that._dataSource.isDisposed()) { |
There was a problem hiding this comment.
The isDisposed() check is redundant because the 'off' method on the dataSource should already handle the case where the dataSource is disposed. The null/undefined check for _dataSource is sufficient. Consider simplifying this condition to just check if _dataSource exists.
| if (!that._dataSource || that._dataSource.isDisposed()) { | |
| if (!that._dataSource) { |
…n updateDataSource method
| dataSource = new PivotGridDataSource(dataSourceOptions); | ||
| } | ||
|
|
||
| that._initEventHandlers(); |
There was a problem hiding this comment.
Calling _initEventHandlers() on every _createDataSource() call creates new bound function references unnecessarily. While this doesn't cause a memory leak (old handlers are properly unsubscribed), it's inefficient.
Consider moving _initEventHandlers() to the constructor and calling it only once. The bound function references can remain the same throughout the DataController's lifetime since they only depend on this context which never changes.
| that._initEventHandlers(); | |
| if (!that._eventHandlersInitialized) { | |
| that._initEventHandlers(); | |
| that._eventHandlersInitialized = true; | |
| } |
| updateDataSource(options) { | ||
| const that: any = this; | ||
|
|
||
| that._unsubscribeFromDataSource(); | ||
| if (!that._isSharedDataSource && that._dataSource) { | ||
| that._dataSource.dispose(); | ||
| } | ||
|
|
||
| that._options = options; | ||
| that._dataSource = that._createDataSource(options); | ||
|
|
||
| if (that._isSharedDataSource) { | ||
| that._handleFieldsPrepared(that._dataSource.fields()); | ||
| that._update(); | ||
| } else { | ||
| that.load(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new updateDataSource method is not covered by tests. Given that this is a critical fix for a memory leak issue, it's important to add test coverage that verifies:
- Event handlers are properly unsubscribed from the old data source
- The new data source is correctly subscribed
- The data source is disposed when it's not shared
- Memory is properly released when switching data sources multiple times
- Both shared and non-shared data source scenarios work correctly
| if (that._dataController) { | ||
| that._dataController.updateDataSource(that._getDataControllerOptions()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The PR description is incomplete. According to the contribution guidelines (CONTRIBUTING.md), the description should clarify:
- What does the PR change? (memory leak fix in PivotGridDataSource)
- How did you achieve this? (reusing DataController instead of recreating, adding updateDataSource method)
- How can we verify these changes? (should include steps to reproduce the memory leak and verify the fix)
This information is important for reviewers and future maintainers to understand the context and reasoning behind the changes.
1 participant
No description provided.