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

Alternative fix for linux race condition between onStart and onExit. #156

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

Conversation

avrecko
Copy link
Contributor

@avrecko avrecko commented May 1, 2024

Continuing from #149.

Moved onStart processing to event processing thread. This way we make sure onStart is called first before any other EPoll event processing is done.

@bturner This is just my suggestion. Curious how you would fix it.

…d caller thread doing onStart.

We moved onStart processing to event processing thread. This way we make sure onStart is called first before any other EPoll event processing is done.
@bturner
Copy link
Collaborator

bturner commented May 5, 2024

@avrecko,

Thanks for the idea, but I'd prefer to keep it where the process manages onStart. The approach I'm leaning toward splits the existing IEventProcessor.registerProcess(T) method to extract a new queueRead (sibling to the existing queueWrite). With the split, registerProcess does the pidToProcessMap and fildesToProcessMap updates and queueRead does the epoll_ctl calls for stdout and stderr (on Linux; it does the kevent calls on macOS). This allows the LinuxProcess code to call registerProcess where it currently does, and then do a myProcessor.queueRead(this) after callStart.

I've opened pull request #158 with that approach, and I've tested it on all of the platforms I can.

@avrecko
Copy link
Contributor Author

avrecko commented May 5, 2024

@bturner

Makes sense to do it this way. Keeping the threading model as is.

Looks good. I'd probably name the new method registerRead or something similar.

I trust the tests are now run with the build process and catch this case.

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.

None yet

2 participants