sonic-id3: fix recursive root listing with artists - #213
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReuses populated child LinkTables during path resolution and adds explicit ID3 state tracking in Sonic XML parsing; artist entries are routed into the active index table and parsing cleanup is improved. ChangesState Management for Link and ID3 Index Tables
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the Subsonic XML parsing logic by explicitly tracking the current index table and reusing existing link tables to prevent incorrect directory listings. It also includes memory cleanup for artist IDs. Feedback highlights a critical thread-safety issue introduced by the use of a static global variable for parser state and a minor style violation regarding missing braces in a conditional block.
| * <index> produces no link or when elements arrive in an unexpected | ||
| * order. Track the parent explicitly via an end-element handler. | ||
| */ | ||
| static LinkTable *id3_current_index_table = NULL; |
There was a problem hiding this comment.
The use of a static global variable id3_current_index_table to maintain state during XML parsing is not thread-safe. In a multi-threaded environment (as suggested by the use of link_lock in src/link.c), concurrent calls to sonic_url_to_LinkTable using the XML_parser_id3_root handler will lead to race conditions and corrupted parser state.
Consider passing a context structure (containing both the root LinkTable and the current index LinkTable) through the void *data parameter of the XML parser instead of using a global variable.
There was a problem hiding this comment.
@svfrd , I am just having a quick look. I am not sure if this is relevant. I will need to have a closer look. But if you think it is not relevant, do let me know.
|
@svfrd, could you address the review comments that the bots flagged up please? Feel free to ignore that stuff that you think are irrelevant. Let me know when this is ready for a proper round of review. There is a pre-commit failure that needs to be fixed too. Let's try and get this merged over the weekend, or by the end of next week. |
| * it instead of unconditionally calling the constructor. | ||
| * | ||
| * Without this, every fs_readdir() on /A would call | ||
| * sonic_LinkTable_new_id3(letter_A->sonic.depth // 0 |
There was a problem hiding this comment.
Please follow the SonarCloud style suggestion.
|
Please update the changelog too. |
|
@svfrd , can you please follow the guideline here and install pre-commit? It is still not passing the pre-commit workflow. https://github.com/fangfufu/httpdirfs/blob/master/src/README.md |
When a Link already has a populated next_table, reuse it instead of unconditionally re-creating it. This prevents memory leaks and unnecessary I/O.
Track the current <index> element explicitly in the Subsonic ID3 XML parser using an end-element handler. This replaces the previous heuristic of using the last link added to the root, which could mis-parent artists if an <index> produced no link or if elements arrived in an unexpected order.
|
|
@svfrd, this is a fairly major bug fix. Thank you for helping out. |
2 participants
path_to_LinkTable() overwrites populated next_table causing recursive root listing under letter dirs.
The path_to_LinkTable() issue affects NORMAL and SINGLE modes too in principle (it always re-creates the table), but those modes' constructors (LinkTable_new with disk cache 1) are idempotent enough that users don't notice.
In SONIC ID3 mode, sonic_LinkTable_new_id3(0, NULL) is destructive, which is why this bug surfaces there.
Tested with funkhwale subsonic endpoint.
Summary by CodeRabbit
Bug Fixes
Performance
Documentation
Summary by CodeRabbit