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

Possible race condition with shutdown() not actually shutting down #1018

Open
kiplingw opened this issue Dec 16, 2021 · 4 comments · May be fixed by #1020
Open

Possible race condition with shutdown() not actually shutting down #1018

kiplingw opened this issue Dec 16, 2021 · 4 comments · May be fixed by #1020
Assignees

Comments

@kiplingw
Copy link
Member

Unless I am mis-reading the code (entirely possible), it appears as though Pistache::Http::Endpoint's shutdown() method can return before all service threads have completed. This method in turn calls SyncImpl::shutdown(). The latter is defined like so:

        void shutdown() override
        {
            shutdown_.store(true);
            shutdownFd.notify();
        }

If this happens, that may create a race condition with some endpoint handlers still executing after the user thinks the endpoint has "shutdown".

I don't know enough about the reactor pattern @oktal employed, but my guess is the behaviour of shutdown() should probably block until it actually has shut down.

@Fabio3rs
Copy link

I think it may happen, the problem I described in this comment seens to be related:
#59 (comment)

@kiplingw
Copy link
Member Author

@oktal, you're likely the only one who can advise us on whether this behaviour was intentional or not.

@dennisjenkins75
Copy link
Collaborator

Can someone create a PR that adds a unit test which reliably reproduces the problem? The PR might not get merged (certainly won't until the bug is fixed), but a reliable repro would be super handy. Its ok it the PR needs to expose/hack/modify the core of pistache in order to rig the failure, since the existing class hierarchy does not lend it self to C++ mocks.

@kiplingw
Copy link
Member Author

Feedback welcome @dennisjenkins75.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants