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

Fix concurrency issue when accessing C2TC #1282

Closed
wants to merge 1 commit into from

Conversation

zeapo
Copy link
Contributor

@zeapo zeapo commented Jan 8, 2021

This ByteArray was mutating itself during the init phase. By moving it to an object, the init {} deals with the initialization and makes it thread safe. The same logic was used for ESCAPE_2_CHAR.

Fixes #1279

This bytearray was mutating itself during the init phase. By moving it to an object, the init {} deals with the initialization and makes it thread safe. The same logic was used for ESCAPE_2_CHAR.
@sandwwraith
Copy link
Member

We do not have the defaultJsonModule anymore, so the problem is not in it — and I'm not sure if mutating this in apply of shared immutable object can lead to sigsegv. Does the provided fix solves the problem for you? If so, can you please add native-specific test for using JSON from different worker threads?

@zeapo
Copy link
Contributor Author

zeapo commented Jan 18, 2021

@sandwwraith yes, this PR fixes the issue. Testing on the 1.0.1 without this patch leads to a sigsev, applying this patch solves the issue.
While debugging, I had to use multiple threads (single thread gives no sigsev) concurrently doing a Json decode to see that the C2TC was null on other threads.

Ok for the tests

@qwwdfsad qwwdfsad self-requested a review January 18, 2021 14:31
@zeapo
Copy link
Contributor Author

zeapo commented Jan 20, 2021

I'm unable to reproduce it from the nativeTests, however from an external Rust caller (I guess in C it will be the same), it's systematic. Would it be enough if I provide you with the Rust code to reproduce it?

@qwwdfsad
Copy link
Member

Would it be enough if I provide you with the Rust code to reproduce it?

Thanks, but I guess we are good without Rust code in the codebase :)

Thank you for your contribution, I'll catch it up from here and will adjust the Gradle script to run tests both in the background and in main K/N thread and will update the PR

@qwwdfsad
Copy link
Member

Closing in the favor of #1301

@qwwdfsad qwwdfsad closed this Jan 20, 2021
@qwwdfsad
Copy link
Member

qwwdfsad commented Jan 20, 2021

Update: this problem couldn't be reproduced with nativeTest because K/N runtime ensures all globals are initialized before main entry point. But it isn't the case for external callers and Obj-C frameworks

@zeapo
Copy link
Contributor Author

zeapo commented Jan 20, 2021 via email

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.

3 participants