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

tests: make pytest start ratbag-emu & better internal api #16

Merged
merged 2 commits into from Sep 20, 2019

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Sep 17, 2019

Signed-off-by: Filipe Laíns lains@archlinux.org

@FFY00
Copy link
Member Author

FFY00 commented Sep 18, 2019

For some reason the server becomes a bit unresponsive in the middle of the test suite, making it sometimes fail. I am not sure what's causing that but it something that should definitely be investigated.

@bentiss @whot any ideas?

Copy link
Collaborator

@bentiss bentiss left a comment

Choose a reason for hiding this comment

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

Can you split out this commit in at least 3:

  • the server start part
  • the udev rule handling
  • the internal API, and explain why this is needed

@bentiss
Copy link
Collaborator

bentiss commented Sep 18, 2019

For some reason the server becomes a bit unresponsive in the middle of the test suite, making it sometimes fail. I am not sure what's causing that but it something that should definitely be investigated.

Have you upgraded to upstream hid-tools? We made a bunch of changes last week that make the test suite pass here.

@FFY00
Copy link
Member Author

FFY00 commented Sep 18, 2019

Have you upgraded to upstream hid-tools? We made a bunch of changes last week that make the test suite pass here.

I just did and it's still the same. After stress testing the server a bit I started getting the following error and the server became slow.

Traceback (most recent call last):
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 2463, in __call__
    return self.wsgi_app(environ, start_response)
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 2449, in wsgi_app
    response = self.handle_exception(e)
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 1866, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/root/.local/lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 2446, in wsgi_app
    response = self.full_dispatch_request()
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 1951, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 1820, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/root/.local/lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 1949, in full_dispatch_request
    rv = self.dispatch_request()
  File "/root/.local/lib/python3.7/site-packages/flask/app.py", line 1935, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/usr/lib/python3.7/site-packages/connexion/decorators/decorator.py", line 48, in wrapper
    response = function(request)
  File "/usr/lib/python3.7/site-packages/connexion/decorators/uri_parsing.py", line 143, in wrapper
    response = function(request)
  File "/usr/lib/python3.7/site-packages/connexion/decorators/validation.py", line 347, in wrapper
    return function(request)
  File "/usr/lib/python3.7/site-packages/connexion/decorators/response.py", line 109, in wrapper
    response = function(request)
  File "/usr/lib/python3.7/site-packages/connexion/decorators/parameter.py", line 126, in wrapper
    return function(**kwargs)
  File "/home/anubis/git/ratbag-emu/ratbag_emu/server.py", line 100, in delete_device
    DeviceHandler.destroy_device(device_id)
  File "/home/anubis/git/ratbag-emu/ratbag_emu/device_handler.py", line 49, in destroy_device
    DeviceHandler.devices[device_id].destroy()
  File "/home/anubis/git/ratbag-emu/ratbag_emu/protocol/base.py", line 340, in destroy
    endpoint.destroy()
  File "/root/.local/lib/python3.7/site-packages/hidtools/uhid.py", line 388, in destroy
    fun()
  File "/root/.local/lib/python3.7/site-packages/hidtools/uhid.py", line 498, in _process_one_event
    self._open()
  File "/root/.local/lib/python3.7/site-packages/hidtools/uhid.py", line 422, in open
    logger.debug('open {}'.format(self.sys_path))
  File "/root/.local/lib/python3.7/site-packages/hidtools/uhid.py", line 336, in sys_path
    return self.udev_device.sys_path
AttributeError: 'NoneType' object has no attribute 'sys_path'

@bentiss
Copy link
Collaborator

bentiss commented Sep 18, 2019

I thought I fixed that error with my latest hid-tools commit.

Basically, we are not pulling the udev events fast enough compared to the other hid and evdev events, meaning that you sometimes pull an event when UHIDDevice is not in a correct state.

What can be a band-aid is to not destroy the device too quickly. Either postpone the destroy at a later time or simply add a little bit of a delay right after the creation in your client. TLDR: I don't think this is something you can easily solve right now, and I really don't have the time today.

@FFY00
Copy link
Member Author

FFY00 commented Sep 18, 2019

Adding a delay did not help that much, I tried up to 1s and this would still happen in some cases. What about giving each device its own dispatch thread?

@bentiss
Copy link
Collaborator

bentiss commented Sep 18, 2019

What about giving each device its own dispatch thread?

That would require a heavy refactoring of hid-tools as the lib has not been designed to handle multiple threads in parallel. I have no immediate solution for you unfortunately

Signed-off-by: Filipe Laíns <lains@archlinux.org>
Signed-off-by: Filipe Laíns <lains@archlinux.org>
@FFY00
Copy link
Member Author

FFY00 commented Sep 20, 2019

the internal API, and explain why this is needed

It's basically the from_mm() stuff, do I really need to explain it now that it has its own commit? Seems pretty straight forward.

@bentiss
Copy link
Collaborator

bentiss commented Sep 20, 2019

It's basically the from_mm() stuff, do I really need to explain it now that it has its own commit? Seems pretty straight forward.

Now it's clear. That's the problem of a gigantic patch: you don't know what to look for when reviewing.

@bentiss bentiss merged commit bf32c93 into libratbag:master Sep 20, 2019
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