Skip to content

Refactor/wasm cstring autoclose#77

Closed
krickert wants to merge 2 commits intoroastedroot:mainfrom
ai-pipestream:refactor/wasm-cstring-autoclose
Closed

Refactor/wasm cstring autoclose#77
krickert wants to merge 2 commits intoroastedroot:mainfrom
ai-pipestream:refactor/wasm-cstring-autoclose

Conversation

@krickert
Copy link
Copy Markdown
Contributor

Per #73 I mentioned that if it was approved, I'll go ahead and wrap the other two malloc/free with tighter exception handling.

This pull request refactors how null-terminated UTF-8 strings are allocated and managed in WASM memory within the Protobuf Java library. The changes introduce a new utility class to handle string memory allocation and deallocation more safely and idiomatically, reducing the risk of memory leaks and improving code clarity. Unit tests are also added to ensure correct resource management.

Memory Management Improvements:

  • Introduced the WasmCStringBuffer class to encapsulate malloc/free for WASM null-terminated strings, ensuring memory is always freed using try-with-resources and AutoCloseable. (core-common/src/main/java/io/roastedroot/protobuf4j/common/WasmCStringBuffer.java)
  • Refactored usages of manual string allocation and freeing in Protobuf.java to use WasmCStringBuffer, replacing the old writeCString helper and updating all relevant methods to use try-with-resources for safer memory handling. (core-common/src/main/java/io/roastedroot/protobuf4j/common/Protobuf.java) [1] [2] [3]

Testing Enhancements:

  • Added a unit test to verify that WasmCStringBuffer correctly frees memory even when exceptions occur, preventing WASM memory leaks across iterations. (core-v4/src/test/java/io/roastedroot/protobuf4j/common/WasmCStringBufferTest.java)

…ateSyntax

Add WasmCStringBuffer (try-with-resources) like WasmInputBuffer for PR73. Removes manual free of filename and export path pointers.
Move WasmCStringBuffer to its own package-private type so v4 can regression-test many throwy iterations against a real WASM instance.
Copy link
Copy Markdown
Contributor

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

Hi @krickert , thanks again for your help on this project!

I see the CI is failing, have you had a chance to check that?

While I appreciate the attempt to tighten the story around malloc and free, this PR introduces additional opt-in constructs like WasmCStringBuffer, which increase the overall complexity.

Specifically, the new class is only used in two call sites. At this stage of the project, I think it’s preferable to accept a bit of duplication rather than lock ourselves into a more opinionated pattern.

@krickert
Copy link
Copy Markdown
Contributor Author

No problem let's just close this one then - I always prefer the auto-closable pattern whenever I see malloc() and free() because it's easy for newbies to cause stupid accidental very very slow memory leaks. But the code works fine and this may have the opposite effect by causing some confusion.

And no prob with the contributions - I feel like there's a lot of potential with grpc but also think every java stack needs to tighten up the edges with serialization here.

@andreaTP
Copy link
Copy Markdown
Contributor

Thanks for understanding 👍

@andreaTP andreaTP closed this Apr 30, 2026
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