Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt at memory fix #1263

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Thrameos
Copy link
Contributor

Fixes #1242

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.95%. Comparing base (7b24a39) to head (c5ffc1d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
+ Coverage   86.90%   86.95%   +0.05%     
==========================================
  Files         113      113              
  Lines       10336    10346      +10     
  Branches     4065     4064       -1     
==========================================
+ Hits         8982     8996      +14     
+ Misses        758      757       -1     
+ Partials      596      593       -3     
Files with missing lines Coverage Δ
native/common/jp_context.cpp 77.89% <100.00%> (+2.33%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Thrameos Thrameos requested a review from marscher March 20, 2025 14:20
@grauchs
Copy link

grauchs commented Mar 20, 2025

I compiled and tested this version (git pull https://github.com/Thrameos/jpype.git jniargs) and the crash reported for jpype-1.5.2 does not happen anymore, i.e. JVMstart() works fine and the issue is fixed.

@Thrameos
Copy link
Contributor Author

@marscher I believe the source of this issue was the incorrect usage of cstr(). To fix it I pushed all of the arguments into a memory block using a copy method, then freed the block at the end. This should be a safer procedure.

I will add a note into the CHANGELOG. But I as I was never able to replicate the issue, I wasn't able to develop a test. I requested a review to make sure I didn't do something silly with the pointer work. If I don't get a review I will likely just included it in the next release as it is a stability issue.

@Thrameos Thrameos added the bug Unable to deliver desired behavior (crash, fail, untested) label Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unable to deliver desired behavior (crash, fail, untested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to start JVM with jpype 1.5.1 on Windows
2 participants