Skip to content

Conversation

@olk
Copy link
Member

@olk olk commented Jan 22, 2021

  • dispatch() might queue the completion handler or invoke it immediately
  • this might result in problems if this code runs in completion
    handlers like coroutines where you must not resume an already running coroutine
  • using post() solves this issue

- dispatch() might queue the completion handler or invoke it immediately
- this might result in problems if this code runs in  completion
  handlers like coroutines where you must resume an already running coroutine
- using post() solves this issue
@gulachek
Copy link

This might fix another issue you found, but this documented issue (196) isn't fixed by the diff I'm seeing. If you see the AsyncReadStream requirements, the async_pipe needs to implement a get_executor member function.

@olk
Copy link
Member Author

olk commented Jan 24, 2021

This patch from above seams to solve our production code.

The problem was that async_coroutine::push() was invoked from sigchld_service::async_wait() while the stackful coroutine was already running.

I refer to the issue #195 not pull-request #196 - my mistake, sorry.

@klemens-morgenstern
Copy link
Collaborator

I think the outer one should be dispatch, but the inner one should post.

@olk
Copy link
Member Author

olk commented Oct 9, 2021

Klemens, I'll try your suggestion at our production code...
But could you give me an explanation why you think that the outer one should call dispatch()?

@klemens-morgenstern
Copy link
Collaborator

The outer just needs to make sure it's on the right executor. I actually think the fix would be to post these invocations:of h.

                    if (pid_res < 0)
                        h(-1, get_last_error());
                    else if ((pid_res == pid) && (WIFEXITED(status) || WIFSIGNALED(status)))
                        h(status, {}); //successfully exited already

klemens-morgenstern added a commit to klemens-morgenstern/boost-process that referenced this pull request Oct 14, 2021
klemens-morgenstern added a commit to klemens-morgenstern/boost-process that referenced this pull request Oct 14, 2021
klemens-morgenstern added a commit to klemens-morgenstern/boost-process that referenced this pull request Oct 14, 2021
klemens-morgenstern added a commit to klemens-morgenstern/boost-process that referenced this pull request Oct 14, 2021
klemens-morgenstern added a commit to klemens-morgenstern/boost-process that referenced this pull request Oct 14, 2021
klemens-morgenstern added a commit that referenced this pull request May 18, 2022
klemens-morgenstern added a commit that referenced this pull request May 19, 2022
Squash after invalid branch & merge conflict.

* Fixed file_descriptor move assignment operator to return a reference to 'this'. Issue # 219

* Returning *this instead of erroneous *this. Issue # 219

* Removed unneeded WNOHANG.

* Closes #190

* Closes #121

* Attempting to fix wchar_t build error on circle.

* Closes #197.

* Changed child(pid_t) signature.

* Multiple fixes.

* Closes #189.

* Closes #191.

* Added missing work guard on windows.

* Trying to catch windows early complete.

* Increased log level on windows.

* Multiple windows test fixes

* Removed overly constraint tests.

* fix missing headers

* Closes klemens-morgenstern/boost-process#218

* Update executor.hpp

explicit cast to int to silence this: `error: non-constant-expression cannot be narrowed from type 'unsigned long' to 'int' in initializer list [-Wc++11-narrowing]`

* Fix posix implementation of move constructor/assignment in file_descriptor

* Adjust docs `@boost` relative paths

* Fixed UB for large environment names.

* Closes #207.

* Drone setup

* Added include for filesystem::fstream.

* Disabled useless tests.

* Fixed environment length checks.

* Pipe test & warning fixes.

* Disabled warnings & added windows include fix.

* More test fixes.

* Removed some tests from apple build.

* Removed some tests from apple build.

* Disabled OSX tests via build script & fixed windows examples.

* TSA fix attempt.

Co-authored-by: James Baker <[email protected]>
Co-authored-by: silent <[email protected]>
Co-authored-by: ikrijan <[email protected]>
Co-authored-by: Shauren <[email protected]>
Co-authored-by: alandefreitas <[email protected]>
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