search

awakesecurity / gRPC-haskell

Haskell gRPC support
https://hackage.haskell.org/package/grpc-haskell
Apache License 2.0
236 stars 74 forks source link

Fix a few memory leaks #140

Closed mrBliss closed 12 months ago

mrBliss commented 1 year ago

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.

ixmatus commented 1 year ago

@mrBliss thank you for submitting this PR! We've been really busy but should have some time soon to review your PR.

fumieval commented 1 year ago

@ixmatus Did you have a chance to take a look at this?

riz0id commented 1 year ago

@fumieval I took a look, theres a few things I need to check but I plan on approving or leaving suggestions today.

ixmatus commented 1 year ago

@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.

fumieval commented 1 year ago

FWIW, we switched to this version in production and it seems to be working fine so far

riz0id commented 1 year ago

@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.

fumieval commented 1 year ago

@riz0id I've just submitted a PR with a branch rebased on top of master: #153

riz0id commented 1 year ago

@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.

fumieval commented 1 year ago

@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 commented 1 year ago

@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.

riz0id commented 1 year ago

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.

riz0id commented 12 months ago

@fumieval I'm closing this and merging the rebased PR.