Event system #14
Event system #14
Conversation
|
Any feedback? |
|
Thanks for the PR, I'm incredibly busy at the moment on other projects I'm trying to push live so don't have the time yet to review these changes as they're quite big. Sorry it's slow. I'll get to them. |
|
Master has since changed so I've created a branch and fixed the conflicts: https://github.com/Jamie-/openvpn-api/tree/event-system |
There was a problem hiding this comment.
I now have more time and have got to reviewing this now.
Good start, thanks for the contribution!
I've created a branch event-system where I've taken the code from here, rebased it onto the new head of master and changed some of the things I've already mentioned in comments.
|
|
||
|
|
||
| def register_callback(callback: typing.Callable[[typing.Type], typing.Any]) -> None: | ||
| _callbacks.append(callback) |
There was a problem hiding this comment.
Ok so callbacks are global, if I have multiple instances of VPN I cannot assign a callback for an event to just one of them, equally, when the callback is called I have no idea which instance they came from.
There was a problem hiding this comment.
Good point, I didn't think about having multiple instances at that time. :)
|
|
||
| from openvpn_api.events import client | ||
|
|
||
| event_types = [importlib.import_module(".client", __name__)] |
There was a problem hiding this comment.
Personally, I think this implementation of different event types (I presume client/server) is a bit gross. You're using a module like a class, what's wrong with an actual class?
By switching to a class you also don't have to do this importlib shenanigans to get all the possible types, you can create a decorator which you apply to all event type classes you have and it stores them in a registry which you can call on later.
There was a problem hiding this comment.
Well, I was more of a module guy than a class guy, but it's changed nowadays. ( Long time from May :D )
I don't exactly remember why I chose module, but I guess it was because of the singleton nature of the event module/class, which would make more sense for it to be a module.
| if not environment: | ||
| raise errors.ParseError("This event type (%s) doesn't support empty environment." % event_type) | ||
|
|
||
| return ClientEvent(event_type=event_type, cid=cid, kid=kid, pri=pri, addr=addr, environment=environment) |
There was a problem hiding this comment.
As said in the first comment, this should all be a class really.
| self._send_queue = queue.Queue() | ||
|
|
||
| self._active_event = None | ||
|
|
There was a problem hiding this comment.
These need type hinting now as mypy checks are enabled in master
| else: | ||
| self._socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| self._socket.connect(self._mgmt_socket) | ||
|
|
||
| self._socket_file = self._socket.makefile("r") |
There was a problem hiding this comment.
From what I've understood so far I don't see why you use the socket in a file-like mode for receiving and not for sending. I'm not sure what benefit you're getting using a file-like object over the regular socket.recv(buff_size).
| @@ -67,6 +93,8 @@ def disconnect(self, _quit=True) -> None: | |||
| if self._socket is not None: | |||
| if _quit: | |||
| self._socket_send("quit\n") | |||
|
|
|||
| self._socket_file.close() | |||
| self._socket.close() | |||
| self._socket = None | |||
There was a problem hiding this comment.
You're spawning threads up but they never get torn down. If we call disconnect, we should also stop the threads
There was a problem hiding this comment.
I partially take that back, they do break when self.is_connected is False but the're no waiting for shutdown in the disconnect method
|
Thank you :) You've already done most of the reviews; Is there anything that I could add to it or should we merge it? |
|
Have also just discovered sockets are not thread safe, so having two threads (a send and a receive) is a "bad thing". Just about to commit a fix for that which puts all socket communication into one thread. But yes, as you say, it does need more unittest testing, and some real life testing, and ideally some load testing too. That would be cool |
|
I discovered that 4096 is a real low buffer size, and the client-connect event never reaches the callback because it gets split up with the low buffer, and an exception gets thrown. maybe we should increase it? It won't even fit a single event, not to mention multiple events coming at the same time. Edit: seems like OpenVPN actually sends one event in multiple packets, and the bug won't be fixed by just increasing buffer size. |
|
I fixed this bug and some others in my repo: HosseyNJF/openvpn-api |
|
I've been using this on a production server for 4 months now, and everything's looking stable, although my fix of the buffer bug isn't waterproof and probably is going to fail sometime. Any updates? |
|
Hi @Jamie-. I know this is an ancient PR, but it would be a waste to abandon this feature :) If you are up for it. I can start testing the |
There is one event class implemented now, the ClientEvent class is handling events coming from client interaction. I wrote two threads for the connection process with sockets; They're working as expected but probably this PR requires much more testing to be mergeable.