jurgenr / aioslsk Goto Github PK

Attempt to use state pattern for transfer state: https://refactoring.guru/design-patterns/state

There isn't a lot of testing for testing the different flows of transfers. This ticket aims to increase the testing, preferably allowing for testing of complex flows

Figure out what the exceptional login cases are and how the server responds:

• Hash mismatch
• Empty password
• Empty username
• Password not matching
• Two clients login for the same user
• Different client versions (?)

Split peer.py functionality into modules:

• search.manager or just move it into search.py : everything related in peer.py related to searching
• distributed.py : everything related to distributed network

There's also other search related things that could move from other things to this new search manager:

• state.py : received_searches and search_requests fields
• server.py : the wishlist task could be moved to the new search module
• server.py : should query, query_room be moved to here as well?

Note: I have yet to understand if there might be an impact: peer.py currently also passes search requests on the children so the distributed.py needs to keep listening for search requests

The Peer messages are half complete and the Distributed messages are not documented at all in MESSAGES.rst

Reconnecting is not working properly because it is currently an infinite loop

When we are without parent and the server starts sending us search requests we should become the root and stop looking. I haven't actually seen this happen but one thing to try is just to never respond to PotentialParents requests. I believe after a timeout the server will just start sending the search requests directly to us.

It is labeled as a bug because I believe currently we don't accept any children if we are root, what will likely happen is that we try to get branch root and level from parent which is None in this case

Currently when a read error occurs on the connection the connection will call disconnect automatically. This gives some issues in combination with the reader loop. For example if the reader loop is cancelled the disconnecting is also cancelled which is not desired. Another issue is that there is currently a race condition when reading a message. When connecting the obfuscated flag gets set to False when the peer init message is received for a distributed or file connection, however in some situations we have already read the header of the next message causing issues.

Perhaps a cleaner way is to let the NetworkManager drive the message reading and if any errors occur to let it handle the disconnecting, although this may also become a dirtier way as we would then need to go through network manager to send/receive a message.

A transfer can currently hold one related task and there's even decisions based on whether there is a task or not. This could lead to issues, example if the other peer connects to upload a file to us and we still have a task running in the background for remotely queuing the transfer (could happen after restart).

Another thing that could be handled is to move the tasks to a separate module which could make it easier to test

Perhaps merge the uploading and downloading state

While writing the USAGE guide on transfers it became clear that there isn't a good way to follow up on transfer progress.

Some ideas for improvement:

• Add a callback when calling client.download or transfer.add_listener
• Just add it to the events
• async iterator?

It should also be possible to track uploads, there already is an event: TransferAddedEvent which includes uploads

Some connections are not properly cleaned up. I believe that this happens when handling the PotentialParents message. When the message is received it attempts to create 10 connections to the potential parents, but when a parent is found all the other tasks get cancelled. Something doesn't get properly cleaned up here.

Type hints are missing in some places or incorrect

Describe possible scenarios where the library would be used, eg. UI environment etc:

• Implement necessary methods for the scenario
• Describe the scenario in the documentation
• Test if possible

Only from a startup point of view.

Settings parameter users.blocked is in the settings currently defines a list of blocked users, however this is never used.

Searches:

• Never return results

Peer connection:

• Block peer and file connections (probably not distributed)

Chat rooms:

• Block private messages
• Block messages in rooms

Find out which path separator is used by the protocol and adapt the code in SharesManager to match it. The remote paths are likely always using backslashes for separators.

It should be possible to pause transfers. The only difference with aborting is that the file will not get deleted

Other clients seem to just leave the connection open until the other peer times out

Issue consists of:

• Rename TransferStorage and SharesStorage to Cache
• When reading from cache, the transfer manager asks to track the user. This should happen somewhere after adding so that the transfers can be read before we are connected to the server
• Create two methods in the client: read_caches and write_caches

The start sequence should be:

• read_caches
• connect

The stop sequence should be:

• disconnect
• write_caches

Some of the SharesManager methods should be made asynchronous

Terms should be removed from the term map in case there are no references left

The variables download_num/downloads needs to be renamed to uploads. There is no message ever sent to the server that would indicate the download number, for uploads however the variable needs to be increased when the SendUploadSpeed is sent.

Currently a lot of method are proxied from the client class to the server manager class. This causes some duplication and it would be better if at least the client methods return the response.

There are issues with this however, foremost that the protocol does not return error messages or returns error messages as private server message. For example JoinRoom should return UserJoinedRoom with the current user in case of success but:

• returns no error message if the room is already joined
• returns an error message as server message in case of for example an error in the name of the room
• the only way to detect error in these cases is to wait for the full timeout

Ideally the protocol would return error messages for each of these. Perhaps an in between approach could be taken and the user can choose:

• Use server_manager to only send the message, it will be up to the
• Use client sends a message and waits for the response, returning timeout after 5 seconds

There isn't much difference between the two, in both cases there's no real error returned, but perhaps for some it would be useful

There's some performance issues when parsing large message (PeerSharesReply). Example, parsing an obfuscated message PeerSharesReply of +- 9.8 MB takes about 16 seconds.

There's some time spent in the zlib library for decompressing, however it still takes a good 13 seconds to parse everything.

len after decompress : 9891693

Clock type: WALL
Ordered by: totaltime, desc

name                                                          ncall            tsub      ttot      tavg
tools/decode.py:44 main                                       1                0.000538  16.47991  16.47991
..sk\src\aioslsk\protocol\messages.py:91 deserialize_request  1                0.000017  13.17655  13.17655
..s\aioslsk\src\aioslsk\protocol\messages.py:995 deserialize  1                0.000908  13.17653  13.17653
..aioslsk\src\aioslsk\protocol\primitives.py:293 deserialize  1                0.001054  13.17562  13.17562
..aioslsk\src\aioslsk\protocol\primitives.py:205 deserialize  437908/1         3.233833  13.13901  0.000030
..aioslsk\src\aioslsk\protocol\primitives.py:148 deserialize  127582/2         0.679876  13.13894  0.000103
tools/decode.py:16 process_obfuscated_message                 1                0.000438  3.267209  3.267209
..ects\aioslsk\src\aioslsk\protocol\obfuscation.py:44 decode  1                1.740596  3.266754  3.266754
..al\Programs\Python\Python38\lib\dataclasses.py:1022 fields  437908           1.780927  2.568208  0.000006
..grams\Python\Python38\lib\dataclasses.py:1045 is_dataclass  1227384          0.979695  2.037089  0.000002
..\aioslsk\src\aioslsk\protocol\primitives.py:83 deserialize  993010           0.710799  1.950954  0.000002
..aioslsk\src\aioslsk\protocol\primitives.py:107 deserialize  244769           0.585590  1.559233  0.000006
..\aioslsk\src\aioslsk\protocol\primitives.py:48 parse_basic  1227388          1.171180  1.555435  0.000001
..\aioslsk\src\aioslsk\protocol\obfuscation.py:30 rotate_key  632946           0.702952  1.021621  0.000002
..sk\protocol\primitives.py:232 _field_needs_deserialization  1227384          0.515529  0.515531  0.000000
..Programs\Python\Python38\lib\dataclasses.py:1037 <genexpr>  1665292          0.508602  0.508602  0.000000
<string>:2 Attribute.__init__                                 310327           0.304919  0.304919  0.000001
..\aioslsk\src\aioslsk\protocol\primitives.py:61 deserialize  117189           0.084166  0.243874  0.000002
..\aioslsk\src\aioslsk\protocol\primitives.py:94 deserialize  117189           0.084617  0.240188  0.000002
..ioslsk\src\aioslsk\protocol\primitives.py:41 decode_string  244769           0.142107  0.222763  0.000001
<string>:2 FileData.__init__                                  117189           0.174014  0.174014  0.000001
..hon38\lib\encodings\cp1252.py:22 IncrementalDecoder.decode  620              0.000709  0.009865  0.000016
<string>:2 DirectoryData.__init__                             10391            0.009081  0.009081  0.000001
..Python\Python38\lib\_bootlocale.py:11 getpreferredencoding  1                0.000005  0.000012  0.000012
..\src\aioslsk\protocol\primitives.py:380 has_unparsed_bytes  2                0.000003  0.000004  0.000002
<string>:2 Request.__init__                                   1                0.000001  0.000001  0.000001
..hon\Python38\lib\codecs.py:260 IncrementalDecoder.__init__  1                0.000001  0.000001  0.000001

Some time spent will be unavoidable, some ideas for improvement:

• dataclasses.is_dataclass : could perhaps be cached?
• dataclasses.fields : the types shouldn't change on the fly and could be cached. in fact a comment in source code of this function already has a comment : Might it be worth caching this, per class? https://github.com/python/cpython/blob/9f89c471fb152fd031791f3c598ceba93d91dcf0/Lib/dataclasses.py#L1029
• obfuscation.rotate_key : some conversion is going on to int and back to bytes. Could perhaps be simplified?

Rename search_queries to search_requests as the object name is SearchRequest

Whenever a scan is complete it should be reported to the server through the SharedFoldersFiles message. This probably needs an internal event to be fired.

It should also be done after login when we load the cache, this currently done manually but needs to come from the shares manager

By default 2 ports are being mapped for incoming connections: a non-obfuscated and an obfuscated one (non-obf + 1). However it is blindly assumed that these ports are available. This ticket should define the behavior if the client is attempted to be started and the port is unavailable.

It should be something like the following:

• Listening port connection should happen before server connection (currently these happen at the same time) so that we can report failures with this port before starting the application
• If the non-obfuscated port fails to connect -> fail (unsure about this decision)
• If the obfuscated port port fails to connect -> log a warning but continue, important here is that we shouldn't tell the server we have an obfuscated port
• Make the obfuscated port optional through the settings
• Obfuscated port should not be mapped through UPNP

The rate limiter should be a variable per connection, not on the network level. Although there's no immediate plans to implement different types of rate limiting (eg.: per download, per user, ...) it would make it a lot easier if those options were to be added.

The only thing supported now is globally, meaning all connections should reference the same rate limiter.

Directories are not properly displayed from other clients when requesting PeerSharesReply. Possibly the same issue applies to PeerDirectoryContentsReply, but actually this has never been verified to work.

Add tests for the distributed connections

Implement locked directories. The shared directories is currently a list but should become an object or a second list should be created.

It should handle locked directories within shared directories and vice versa.

User should be able to decide to who to share

Use type generics instead of inheriting from future

Publish the documentation to either ReadTheDocs or GitHub Pages (or both)

Currently the constructor for client.py is huge because it needs to initialize all the managers. It's not very flexible either because different managers depend on each other.

There needs to be a better way to do this so that users can implement their own behaviour

Factory pattern could help perhaps. The only problem is dependencies between the managers

There's a bunch of warnings when running the tests cluttering the output. Presumably because there are certain async functions not awaited

Currently the test results are all lumped together. They should be per python version (perhaps as a suite)

Make a release pipeline for tags

There's no methods or events for requesting the recommendations currently, it's unclear if this system still works but it needs to be added.

Most of the message definitions for it exist but at least GetGlobalRecommendations is missing.

Investigate exceptional cases for chat rooms:

Joining/Creating

Creating an existing public chatroom

Joins the existing public room

Creating a public room that exists as private room

• Message with 0x03EB (room name)
• Message from server: "The room you are trying to enter (<room_name>) is registered as private."

Creating a private room that exists as public room

Joins the existing public room.

In one case a message from server was seen. This might happen if the room was created previously as public and no-one is inside. The room was joined anway:

"Room (<room_name>) is registered as public."

Join a private room which you are not part of

• Message with 0x03EB (room name)
• PrivateMessage from user 'server' (is_admin=True): "The room you are trying to enter (<room_name>) is registered as private."

Joining a room which the user is already part of

No response

Empty name

• PrivateMessage from user 'server' (is_admin=True): "Could not create room. Reason: Room name empty."

Sending messages

Send message to public/private room which you are not part of

Nothing happens

Private Room

Adding users

Add user to a private room that does not exist (room)

No response

Add user to private room but user has private rooms disabled

Message: "user <user_name> hasn't enabled private room add. please message them and ask them to do so before trying to add them again."

Add user to private room but user does not exist

Message: "user <user_name> is not logged in."

Add user to private room but user is not online

Message: "user <user_name> is not logged in."

Add user to a private room which he is already part of

Message: "user <user_name> is already a member of room <room_name>"

Add user without having the correct permissions

Nothing

Add user empty room name

Nothing

Add user empty username

Message: "user <user_name> is not logged in."

Add user while not being the room to invite

Works, user automatically also joined room?

Can only owners invite or operators as well?

Removing users

Not joined

Works

Not a member

Nothing

Empty room / user

Nothing

Remove user, as operator, other operator

Nothing

Remove user, as operator, owner

Nothing

Grant Operator

Empty username

Message: "user <user_name> is not logged in."

Empty room name

Nothing

Revoke Operator

Revoke from another operator

Nothing

Drop membership

Not part of room / empty room name

Nothing

Public room

Nothing

Move the documentation to a docs directory and fix reference to the messages

• There is a logic error in this method
• It could be used in _handle_connect_to_peer
• Add tests for it

Limit the access to certain objects and functions in the library and shorten the way functions and classes can be imported

Modify the __init__.py to allow shorter imports example: instead of having to write from aioslsk.transfer.manager import TransferManager to from aioslsk.transfer import TransferManager

Look at __all__ on limiting imports

There is a max amount of children we can have and in case our upload speed is lower than the received min speed we should not accept children.

• Figure out how min speed is determined. Is it locally calculated or is the one used from the server?
• When using SendUploadSpeed does the server send an AddUser or GetUserStats to all tracking users? This is important because the client automatically performs an AddUser for the logged in user, is it based on this?

Add room and user search: figure out how these searches are received:

• Make necessary changes
• Document

Remove unnecessary dependencies and update the Python version to support 3.11

The difference and usage of the Configuration and Settings is not clear and could be improved

The current namings and structure might not be ideal

• model
• peer
• server_manager -> server
• slsk -> client
• transfer
• shares -> share (?)

Perhaps it would be better to place these under separate packages

• server
  ** model (what is currently called model)
  ** manager
• transfer
  ** model
  ** manager
• peer
  ** model
  ** manager

The length should still be serialized if a string is mandatory but None is passed

Missing some method for privileges, as well as some events

Can only be done once the project is public

Attempt to guarantee that the status stored locally is correct. There are a few situations where it might not be a 100% reflection of reality.

Example, the server seems to send user status updates to users in the room. But once we leave the room we can no longer be sure of the status (unless we are also tracking it through AddUser). Perhaps it is best to understand how (but maybe also what is tracked from the other users).

Possibly there should be a flag set with info on how the user is currently tracked.