Conversation
zmerp
force-pushed
the
unified-sockets
branch
from
d8eae20 to
03f29f8
Compare
The-personified-devil
left a comment
The-personified-devil
left a comment
There was a problem hiding this comment.
Got too tired to understand the sockets part but didn't wanna lose my review progress, will take a proper look at that tmrw
| lifecycle_state, | ||
| proto_socket, | ||
| socket, | ||
| connection_result, |
There was a problem hiding this comment.
I think the error case of connection_result should be handled directly here already, tho that might mean needing to repackage the data from the accepted case into it's own struct.
There was a problem hiding this comment.
Yeah I can do. It's all a matter of code formatting, if to include this into the nested call or not
There was a problem hiding this comment.
It seems it's more unergonomic to handle the result before calling connection_pipeline because we would need to pass more variables around. In any case I moved this as the first thing done in connection_pipeline.
There was a problem hiding this comment.
That's why I mentioned you might need to repackage it into a struct. But passing around a result is really weird. (I'll allow it if you fix it in a later pr tho)
There was a problem hiding this comment.
That is probably fixed after v21. Because we will remove the "standby" state. So that will not be called "result" anymore
| // to thos client, no other client can connect until handshake is finished. It will then be | ||
| // temporarily relocked while shutting down the threads. | ||
| let mut session_manager_lock = SESSION_MANAGER.write(); |
There was a problem hiding this comment.
This lock is partly worthless since we're already starting the connection beforehand...
Tho I'm pretty sure it doesn't actually matter because we seem to still only read config stuff after the lock is acquired, but I'm not really sure
There was a problem hiding this comment.
This RwLock is released using a function I crafted, alvr_common::wait_rwlock which mirrors the API for Mutex. The purpose of locking the session is to make sure the same device is not attempted more than one connection at the same time. You may have a point, we should lock the session before even acquiring a socket connection, although it's not possible to send a locked RwLock or Mutex across threads
There was a problem hiding this comment.
I tried and unfortunately it doesn't work. in any case yeah, everything except the initialization fo the connection is done after the lock is acquired
There was a problem hiding this comment.
Same here, it's fine for the time being if you fix it in a later pr
There was a problem hiding this comment.
I literally can't fix this. It's prohibited by Rust aliasing rules, to send a lock across threads
|
Address the other comments and I'll let the actual socket mess slide for the time being. |
|
I made a mistake in a comment: PR (2) will be about porting the client to SocketConnection, (3) will refactor the SocketConnection internal in a non-breaking way, and (4) will be breaking. Still, after this PR I will be able to work on the deferred HMD initialization. |
|
I'm going to delay this PR until after the deferred HMD initialization PR |
2 participants
This is the first of a series of PRs to refactor and merge the control socket and stream socket. Like the tracking rewrite, it will not be 100% completed before making breaking changes for v21. This PR also includes some refactoring in connection.rs and a fix for glitchy restarting where the client would temporarily reconnect to the server while it's restarting.