Skip to content

Conversation

@btovar
Copy link
Member

@btovar btovar commented Dec 9, 2025

I saw these leaks only with recovery tasks.

Merge Checklist

The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.

  • make test Run local tests prior to pushing.
  • make format Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)
  • make lint Run lint on source code prior to pushing.
  • Manual Update: Update the manual to reflect user-visible changes.
  • Type Labels: Select a github label for the type: bugfix, enhancement, etc.
  • Product Labels: Select a github label for the product: TaskVine, Makeflow, etc.
  • PR RTM: Mark your PR as ready to merge.

Copy link
Member

@dthain dthain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch! But it seems to me that an insert of a duplicate key is a bigger problem than the leak of the table entry, it is also possibly the leak of the contained object too.

Do we need a notice() here to indicate the implied loss of an object at that key? Or maybe check to see if it is the same object and key?

@btovar
Copy link
Member Author

btovar commented Dec 9, 2025

That would be a good idea! As it is right now there is the memleak of the actual object value as the caller assumes it is always inserted.

@dthain
Copy link
Member

dthain commented Dec 9, 2025

Or perhaps hash_table_insert should return the prior value (or null) at that key?

No point in returning zero on memory allocation failure -- those should be calling xxmalloc instead.

@btovar btovar merged commit 9d3baf2 into cooperative-computing-lab:master Dec 11, 2025
32 of 36 checks passed
@btovar btovar deleted the memleaks branch December 11, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants