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

Restructure the project for new versioning approach #116

Merged
merged 5 commits into from Jun 11, 2023

Conversation

ItsDrike
Copy link
Member

@ItsDrike ItsDrike commented Jun 8, 2023

This PR resolves #45, and restructures the entire project to no longer support multiple minecraft protocol versions, moving to a single (latest) version model.

Note that this is a completely breaking change, and there will NOT be a deprecation period. This is because of how this change impacts the rest of the project development. As this project is still in pre-release stage, deprecation here would greatly slow down any further development here, forcing contributors to maintain both the temporary deprecation layer, and the new single-version one.

For more details on how this change will affect the project going forward, and why it is necessary, I'd suggest checking out #45, containing a detailed explanation.

I have updated the README examples which now use this new version, so I'd suggest checking that out before moving forward with trying out this version, or reviewing the code, as it demonstrates how this new approach will work. Note that when testing this version out, it will almost certainly break any existing code-bases using mcproto, checking the README changes and looking at the changelog fragment message should make it clear on how to update.


Some decisions I made here, which I find potentially worth considering during a review. I'm open to doing these differently:

  • The generate_packet_map function is dynamic - imports the appropriate libraries on function run, depending on the requested packets. The alternative here would be to simply hard-code packet maps dictionaries as constants. This could be done either in the new packet_map.py file, but also in the individual game state folders for the packets, such as mcproto.packets.login import CLIENTBOUND_MAP.
  • The generate_packet_map uses caching, to avoid needlessly re-importing the modules and walking over all of the packets again. The clear downside of this is the fact that it uses more memory, though that's almost irrelevant as packet maps are relatively small.
  • Because of how caching works, since we're returning a mutable dictionary from generate_packet_map, the cache would hold the same reference as the returned dict, potentially causing issues if the user modifies that returned dict. For this reason, I decided to also add another decorator, responsible for copying the result afterwards, making sure that the cache holds a different instance of the dictionary, and what users see is simply a copy of this instance, which they can modify or do whatever they wish with, without risking breaking future calls to this function. For this reason, the function will now only return a mapping proxy, which is immutable, and holds a strong reference to the dict internally (as suggested by @Martysh12).

@ItsDrike ItsDrike added p: 0 - critical This needs to be addressed ASAP z: breaking This introduces backwards compatibility breaking changes to the public API t: rewrite Complete or partial rewrite of something a: packets Related to packets (packet classes, or their reading/writing) a: API Related to exposed core API of the project labels Jun 8, 2023
@ItsDrike ItsDrike force-pushed the versioning-restruture branch 2 times, most recently from 4fa3568 to df998f0 Compare June 9, 2023 19:42
mcproto/packets/packet_map.py Show resolved Hide resolved
mcproto/packets/packet_map.py Show resolved Hide resolved
In issue #45, it was decided that mcproto should not end up supporting
multiple protocol versions, and instead it should only deal with the
single (latest) version.

This commit therefore restructures the project, removing all versioned
components, keeping only a single version.

This change also means a removal of the VersionMap ABC, which was used
for PACKET_MAP construction for the various game states. As this is no
longer the case, the current approach for now will be to construct the
packet maps manually, which is indeed very annoying, and a better
approach will need to be implemented later.
Before this change, if the user called `generate_packet_map`, a dict
object would get cached, and that same object (reference) would be
actually returned. This means that when the fucntion would get called
again, the same cached dict would be returned.

Generally, this wouldn't be an issue, however if the user modifies this
returned dictionary, since caching only stores the very same reference,
on next call the modified version of that dict would be returned again.

This was mentioned in the function's docstring as a warning, however I
don't see that as sufficient, as people will usually do stupid things
with libraries, without reading the docs, which in this case, would
potentially end up breaking a lot of things.

To avoid this issue, another decorrator was created, responsible for
copying the returned object after the cache decorator. That means the
cached version will hold a dict that no user will be able to modify
accidentally, as the returned dict will simply be a copy of it.

This does mean using slightly more memory, but it's a pretty small
amount, and it's well worth the potential mess-ups this prevents.
@ItsDrike ItsDrike marked this pull request as ready for review June 11, 2023 09:38
Copy link
Contributor

@Martysh12 Martysh12 left a comment

Choose a reason for hiding this comment

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

That looks good to me, but that last point bugs me a little.

  • I like the idea of importing packets dynamically, as maintaining a big dictionary of packets does seem like a pain.
  • I don't mind the memory overhead caching gives the library, because importing is a slow procedure.
  • Is it possible to return some kind of "immutable dictionary", so that you don't have to copy the dictionary? Maybe MappingProxyType from the types module? (available since Python 3.3)

@ItsDrike
Copy link
Member Author

ItsDrike commented Jun 11, 2023

Is it possible to return some kind of "immutable dictionary", so that you don't have to copy the dictionary? Maybe MappingProxyType from the types module? (available since Python 3.3)

This is a nice idea, however you need the real dictionary to be kept somewhere, and since a function call creates a new stackframe, with all of the variables in it, as the function ends, that stackframe gets deleted, and that means taking all of the local variables it held alongside. A mapping proxy needs a reference to an actual dict object, so we can't have that dict going out of scope, while the mapping proxy is returned.

You could probably do something hacky, like crating a global variable, holding a list of dictionaries, which are the return values, so that the caching then returns that mapping proxy back, with the actual dict being in that global variable list. But this is probably way too convoluted and I'm not sure I love that.

I do also find this caching with copying pretty annoying though, I just wasn't able to think of anything better. Btw, do you think the name utils/decorators.py works well here? Or can you think of a better name? I didn't want an overly specific name just for this copying logic, to avoid too many files in utils dir, but decorators might be a bit too broad. Not sure where else to put it / what to call that file.

@ItsDrike
Copy link
Member Author

Hmm, actually after some testing, it seems like this would work, as MappingProxyType does count as an object holding a direct reference to the original dict, so it's more like returning a class holding a dict inside of it. I assumed mapping proxy worked more like a weakref. In this case I would agree that just returning a mapping proxy would probably work pretty well.

Copy link
Contributor

@Martysh12 Martysh12 left a comment

Choose a reason for hiding this comment

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

This is a nice idea, however you need the real dictionary to be kept somewhere, and since a function call creates a new stackframe, with all of the variables in it, as the function ends, that stackframe gets deleted, and that means taking all of the local variables it held alongside. A mapping proxy needs a reference to an actual dict object, so we can't have that dict going out of scope, while the mapping proxy is returned.
You could probably do something hacky, like crating a global variable, holding a list of dictionaries, which are the return values, so that the caching then returns that mapping proxy back, with the actual dict being in that global variable list. But this is probably way too convoluted and I'm not sure I love that.

Riiight. Oh well, copying is not that bad after all.

Btw, do you think the name utils/decorators.py works well here? Or can you think of a better name? I didn't want an overly specific name just for this copying logic, to avoid too many files in utils dir, but decorators might be a bit too broad. Not sure where else to put it / what to call that file.

I can't think of a better name than utils/decorators.py, so I guess it's final.

I'll approve the changes, then.

ItsDrike added a commit that referenced this pull request Jun 11, 2023
This reverts commit af54ac1.

Following a [suggested change][suggested-change], this moves to using
immutable `MappingProxyType` as return values, bypassing the need for
copying.

[suggested-change](#116 (review))
ItsDrike added a commit that referenced this pull request Jun 11, 2023
This reverts commit af54ac1.

Following a [suggested
change](#116 (review)),
this moves to using immutable `MappingProxyType` as return values,
bypassing the need for copying.
ItsDrike added a commit that referenced this pull request Jun 11, 2023
This reverts commit af54ac1.

Following a [suggested change](#116 (review)),
this moves to using immutable `MappingProxyType` as return values,
bypassing the need for copying.
This reverts commit af54ac1.

Following a review comment: #116 (review),
this moves to using immutable `MappingProxyType` as return values,
bypassing the need for copying.
@ItsDrike ItsDrike merged commit 3295e48 into main Jun 11, 2023
12 checks passed
@ItsDrike ItsDrike deleted the versioning-restruture branch June 11, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: API Related to exposed core API of the project a: packets Related to packets (packet classes, or their reading/writing) p: 0 - critical This needs to be addressed ASAP t: rewrite Complete or partial rewrite of something z: breaking This introduces backwards compatibility breaking changes to the public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide on final versioning structure
2 participants