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

Prevent race condition issues when parsing types #1352

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PaulTaykalo
Copy link

@PaulTaykalo PaulTaykalo commented Jul 22, 2024

Long story short - sourcery is crashing sometimes on processing files in our product
This is mostly because of the multithreaded access to the ParserResultsComposed which is responsible for resolving types.
Since that class has a somewhat cache (when using / creating actualTypeNames and checking if they're there), this can lead and leads to the situations, when TypeName being overreleased

This is more hack than fix since technically, this makes type resolving almost serial.

In any case:

  • This fixes crash on our project
  • This removes all runtime issues about data races

While, this is, probably, not the best solution, PR is here, so this can be a hack.
P.S. Alternative is to turn on --serial option.

image

@PaulTaykalo PaulTaykalo force-pushed the fix/sourcery-multi-theaded-access branch from 7ec8afd to ec8f5c9 Compare July 22, 2024 06:01
@PaulTaykalo PaulTaykalo force-pushed the fix/sourcery-multi-theaded-access branch from ec8f5c9 to 5029e6f Compare July 22, 2024 12:34
@pavel-trafimuk
Copy link
Contributor

pavel-trafimuk commented Jul 22, 2024

My 5 cents

  1. maybe move lock inside the function, not outside, as it could happen everywhere in project.
  2. I don't like NSLock cause it's too slow, but I don't know of unfair lock availability on different OSes.
    Anyway thanks for your supporting of Sourcery

@art-divin
Copy link
Collaborator

Hey @PaulTaykalo ,

while this is a reasonable fix and workaround, it can hit pretty bad in terms of performance.
I am not saying that it will, but I would avoid a workaround.

Instead, I would implement a different mechanism for storing shared state, or even better - avoid the shared state completely.
Though I do not recall how Composer is invoked or structured to the level of precision allowing do the right choice in the assumption if it's possible or not. Needs an investigation and POC.

But generally speaking ,there's nothing which stops Sourcery from using async/await instead of this custom parallelization mechanism. A little re-write, yes, but modern and robust.

@PaulTaykalo
Copy link
Author

@krzysztofzablocki Yep, sure. Totally understand. This PR is more like issue with a hot-fix.
Feel free to close it.

P.S. Would propably dig deeper, but for some reason Xcode doesn't show live issues on the Sourcery project, which hugely slows down development process. Are you experiencing this as well? Or Are you using other editors/IDEs
image

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