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

TST: increase barrier wait time due to travis slowness #346

Merged
merged 16 commits into from
Nov 14, 2018

Conversation

klauer
Copy link
Member

@klauer klauer commented Oct 8, 2018

Based on what I've seen locally with barrier breakage using 100 to 300+ threads, it seems that the time needed to complete these tests on travis is significantly greater than when run locally. Let's see if this makes multithreaded tests pass more repeatably.

@codecov-io
Copy link

codecov-io commented Oct 8, 2018

Codecov Report

Merging #346 into master will increase coverage by 0.22%.
The diff coverage is 95.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #346      +/-   ##
==========================================
+ Coverage   90.98%   91.21%   +0.22%     
==========================================
  Files          95       95              
  Lines       15077    15079       +2     
==========================================
+ Hits        13718    13754      +36     
+ Misses       1359     1325      -34
Impacted Files Coverage Δ
caproto/_state.py 98.36% <ø> (ø) ⬆️
caproto/_broadcaster.py 98.68% <100%> (+3.39%) ⬆️
caproto/tests/test_threading_client.py 96.39% <93.33%> (+1.79%) ⬆️
caproto/threading/client.py 90.13% <95.55%> (+0.4%) ⬆️
caproto/tests/test_pyepics_compat.py 98.65% <0%> (+0.16%) ⬆️
caproto/_circuit.py 97.95% <0%> (+0.29%) ⬆️
caproto/server/common.py 85.18% <0%> (+0.38%) ⬆️
caproto/asyncio/server.py 90.17% <0%> (+0.44%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b6e2f5...887cb31. Read the comment docs.

klauer and others added 7 commits November 12, 2018 14:39
Reasons for removal:
- We have been circumventing this check in an ugly way, flipping the
state on command *composition* rather than on send. Might as well just
remove it rather than circumvent it.
- It's a half-measure, enforcing that we *attempt* registration but not
saying anything about whether it worked.
- Registration is really a best practice, not a core part of the
protocol.
- The implementation was a wart, not using the state machine like
everything else.
@danielballan
Copy link
Collaborator

One build passed. Each of the other 3 failed one or more variant of
caproto/tests/test_threading_client.py::test_multithreaded_many_subscribe

caproto/threading/client.py Outdated Show resolved Hide resolved
Copy link
Member Author

@klauer klauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Seems like this should fix the double-sub issue.

cb_id = self._callback_id
self._callback_id += 1
self.callbacks[cb_id] = ref
return cb_id

def remove_callback(self, token):
with self._callback_lock:
with self.callback_lock:
self.callbacks.pop(token, None)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pop is atomic

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you do not want this to fire while something else is iterating over the callbacks etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

Example:

```
[W 16:57:53.396 client:1294] Invalid command EventCancelResponse(data_type=<ChannelType.LONG: 5>, sid=1, subscriptionid=1, data_count=0) for Channel <ClientChannel name='f13dcfe9:int' cid=0 sid=1 data_type=5 logger_name='caproto.ch.f13dcfe9:int.0'> in state <ChannelState states={CLIENT: CLOSED, SERVER: CLOSED}>
    Traceback (most recent call last):
      File "/home/dallan/Repos/bnl/caproto/caproto/threading/client.py", line 1288, in _process_command
        self.circuit.process_command(command)
      File "/home/dallan/Repos/bnl/caproto/caproto/_circuit.py", line 200, in process_command
        self._process_command(self.their_role, command)
      File "/home/dallan/Repos/bnl/caproto/caproto/_circuit.py", line 309, in _process_command
        transitions = chan.process_command(command)
      File "/home/dallan/Repos/bnl/caproto/caproto/_circuit.py", line 491, in process_command
        self.states.process_command_type(role, type(command))
      File "/home/dallan/Repos/bnl/caproto/caproto/_state.py", line 310, in process_command_type
        self._fire_command_triggered_transitions(role, command_type)
      File "/home/dallan/Repos/bnl/caproto/caproto/_state.py", line 288, in _fire_command_triggered_transitions
        raise err from None
    caproto._utils.RemoteProtocolError: <ChannelState states={CLIENT: CLOSED, SERVER: CLOSED}> cannot handle command type EventCancelResponse when role=Role.CLIENT and state=States.CLOSED
```
@tacaswell tacaswell merged commit 4a5c997 into caproto:master Nov 14, 2018
@klauer klauer deleted the tst_multithread_increase_time branch May 10, 2019 16:28
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.

4 participants