gRPC-haskell
gRPC-haskell copied to clipboard
awakesecurity
Fix a few memory leaks
This fixes the memory leaks identified in #137 and adds a workaround for another memory leak I don't fully understand. See the commit messages for more details.
@mrBliss thank you for submitting this PR! We've been really busy but should have some time soon to review your PR.
@fumieval I took a look, theres a few things I need to check but I plan on approving or leaving suggestions today.
@fumieval I took a cursory look but I'm just not familiar enough with this C-code to do a good job reviewing it, generally it seems fine though. A more experienced C-programmer looked at it as well and concluded they would need to swap in a lot more context about how the C-code works before being able to leave comments. I unfortunately don't have time to do that (and I don't think they do either).
Hopefully @riz0id does have the time but I do think we should be careful, especially with workarounds for a memory leak the original author didn't fully understand in 23b114e.
FWIW, we switched to this version in production and it seems to be working fine so far
@fumieval is it fine if I merge master or could you? I still need to dig into this but master has some fixes to CI that I think are affecting this branch.
@riz0id I've just submitted a PR with a branch rebased on top of master: #153
@fumieval Thank you, I'm sorry I've been so slow with this. I've been really busy and theres still some CI issues on OSX we've been dealing with lately. I'm trying my best to find some time to review this. Thanks for being patient.
@riz0id No problem as we can always use a forked version.
There's a problem on OSX environment indeed https://github.com/cachix/install-nix-action/issues/183
@riz0id No problem as we can always use a forked version.
There's a problem on OSX environment indeed cachix/install-nix-action#183
Yeah, I've been dealing with that a lot lately. Waiting to see if #154 resolves the issue.
Okay seems like upgrading to install-nix-action@v22 fixed the environment issue. I'll merge that fix to master so that you can merge it into #153. Your branch should pass CI after that.