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

test_processbus.py::TestBusMethod::test_wait flakily fails with START_ERROR because of EINVAL #16

Open
webknjaz opened this issue Mar 19, 2021 · 1 comment
Labels

Comments

@webknjaz
Copy link
Member

webknjaz commented Mar 19, 2021

Huh?

The test in question is unstable and it's mostly visible in Windows runs but I think I also saw similar Ubuntu failures.

Example test failure: https://github.com/cherrypy/magicbus/runs/2114870045#step:12:106

The log appears to be:

Bus state: ENTER
Bus state: IDLE
Bus state: START
Bus state: START_ERROR
Exiting due to error in start listener:
Traceback (most recent call last):
  File "D:\a\magicbus\magicbus\magicbus\base.py", line 205, in _transition
    os.write(write_fd, b'1')
OSError: [Errno 22] Invalid argument
Bus state: STOP

Pointers

magicbus.base.Bus.wait() creates a pipe for getting the transition notifications more efficiently and adds it to the instance attribute _state_transition_pipes: https://github.com/cherrypy/magicbus/blob/57d151e/magicbus/base.py#L315-L334
This only happens if the runtime is select-compatible.
It then enters a loop waiting for either the select.select() to get a notification about any transition change or just sleeps for a while otherwise.

magicbus.base.Bus.transition() (through magicbus.base.Bus._transition()) writes a byte into every pipe in _state_transition_pipes attempting to notify the waiters before their timeouts expire: https://github.com/cherrypy/magicbus/blob/57d151e/magicbus/base.py#L202-L205

Performing the transitions in separate threads may create a race condition when list(self._state_transition_pipes) got a copy of the pipes, then during the iteration, some pipe file descriptors get closed and removed by wait() asynchronously between that copy and the loop iteration attempting to write into the previously copied descriptors.
This probably means that the hack of avoiding the "Set changed size during iteration" is not clever enough and we need to suppress the error that happens there.
One thing that is surprising to me is that I cannot reproduce this behavior under GNU/Linux on my machine — it always returns other errors (like "Broken pipe" if the reader end of the pipe has been closed before writing or "Bad file descriptor" if the write fd has been closed). Does Windows produce different errors?

Another thing to consider is that when the fd is closed, a slightly different race may still be possible — when a new pipe has been opened and the kernel allocated exactly the same file descriptor that the old discarded pipe had but is still in the variable. It's not that bad if we just hit another pipe created by wait() but may be dangerous if that file descriptor is pointing at something else completely. We should probably introduce some locking mechanism to prevent this.

5d4cfe2 should help us see the future exceptions since they weren't logged properly before.

Refs I encountered when I was trying to explore how write() could cause EINVAL:

Weird stuff

I was trying to reproduce this specific type of OSError on my Gentoo machine under IPython by using os.pipe() and trying to break the file handles but never succeeded to trick it into returning EINVAL (OSError: [Errno 22] Invalid argument). I haven't tried adding threads, though.

@aminusfu do you have any insight into this?

Related archeology:

@webknjaz webknjaz added the bug label Mar 19, 2021
webknjaz added a commit that referenced this issue Mar 20, 2021
webknjaz added a commit that referenced this issue Mar 20, 2021
This should theoretically get rid of one possible exception ("Broken
pipe") happening in `Bus._transition()` due to a race condition.

Ref #16
webknjaz added a commit that referenced this issue Mar 20, 2021
webknjaz added a commit that referenced this issue Mar 20, 2021
This should theoretically get rid of one possible exception ("Broken
pipe") happening in `Bus._transition()` due to a race condition.

Ref #16
@webknjaz
Copy link
Member Author

I've added the basic suppression in #17 but keeping this open until there's a better locking mechanism in place.

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

No branches or pull requests

1 participant