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

Add: pass upload date to OpenTTD client #43

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

perezdidac
Copy link

@perezdidac perezdidac commented Mar 26, 2021

Implementing logic to pass the "upload-date" field to OpenTTD so we could explore the ability of displaying it in the NewGRF window as well as sorting by upload date in the client as well.

I have tested it by making the corresponding changes in OpenTTD to receive the value, and here it is:

image

In particular, I've focused on making sure this doesn't break older versions. Here is what I have successfully tested:

  1. Bananas returning the new field, OpenTTD not knowing about it -> OpenTTD ignores it as it has read the latest field it knows about it (tags) and moves on to the next NewGRF. No memory leak or anything out of normal.
  2. Bananas returning the new field, OpenTTD knowing about it -> OpenTTD reads it successfully.
  3. Bananas not returning the new field, OpenTTD expecting it -> OpenTTD tries to read it but does not actually read it. I used CanReadFromPacket() to verify and make it safe in case we have to rollback Bananas and newer OpenTTD clients still want the value.

@perezdidac perezdidac force-pushed the booboo branch 2 times, most recently from 7c7b1aa to 8993390 Compare March 26, 2021 21:03
@TrueBrain
Copy link
Member

Note: I have no idea if this will work or it will blow everything up since I haven't been able to successfully run bananas-server locally, but at least I can get some attention from @TrueBrain on this :)

You make it sound like I am impossible to reach! You can always poke me on Discord (or IRC) or whatever .. just try TrueBrain indeed; I don't normally respond to TrueNorth :D :P

You haven't mentioned what issue you run into starting this locally; it should "just work", so I am curious what doesn't work. But possibly best to talk this over on Discord, I think :) So poke me there with where ever you are stuck!

As for this PR, the short answer: it is complicated. The longer:

Where we made all the other parts of the protocol contain a "protocol version", for some reason we completely forgot to add this in the BaNaNaS part of the protocol. In other words: making changes to the protocol like you propose here, would break all older clients instantly. And as we like that even 1.4 (or 0.7) can still use the Online Content Service, that is not ideal :)

This means before we can extend the protocol, we somehow need a method that older clients can still connect while newer can too. There are two approaches to this problem:

  1. find a way to add a version to the protocol. This can be done by a new packet, or for example using some bits from the "content_type" field (to name something hacky). I have not given it any real thought, honestly.
  2. rework the BaNaNaS protocol to, for example, be JSON based. We have been wondering about this for a while now. Additionally, we might even want to make it completely HTTPS based (by adding libcurl to the client), and ditch the custom protocol here. That would instantly fix so many issues. Of course older clients still use the custom protocol, just newer no longer won't.

Before we can extend the protocol, either of the above should be implemented. 1) is relative lightweight, just needs a clever design. 2) takes a lot more effort, but is more future-proof. Nobody is working on either.

Otherwise, if we would have something like above in place, your PR seems fine.

That all said, if you just want to experiment a bit, your PR will be just fine. It allows you to get the info in the client so you can see how it looks and feels, and find out if this is a good idea at all :) It just ... can't go to production for now.

As I said: complicated :D But we have to tackle this problem sooner or later, so if you are up for it, I am here to help!

@perezdidac
Copy link
Author

Thanks for your lovely response. That's the reason I added the new field at the end of the packet, since I looked at the response debugging OpenTTD and yes it looks like field come in a sequence one after the other. Are you sure the extra packet of data at the end will hurt clients?
The option (1) you propose is more short term and seems doable. Option (2) is the way to go IMO, but yeah it will require more work.
BTW the issue I am facing when running it locally is: https://pastebin.com/raw/xNU0kySa ValueError: reuse_port not supported by socket module
It seems that reuse_port does not work on Windows maybe

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

You are correct that old clients will be very forgiving .. which is a bug on its own, I would say :D That does mean this is pretty hacky, but .. the effort the other solutions take is much greater.

So, I do not have a problem with this PR. The PR in OpenTTD I leave to another dev to evaluate what he thinks of it etc. If that is approved, we will make this happen. We just have to be careful in what order we merge what :) But that will be fine!

bananas_server/helpers/datetime_format.py Outdated Show resolved Hide resolved
@@ -151,6 +155,7 @@ def _read_content_entry_version(self, content_type, unique_id, data, md5sum_mapp
size += 1
for tag in data.get("tags", []):
size += len(tag) + 2
size += len(upload_date) + 2
Copy link
Member

Choose a reason for hiding this comment

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

We need to check if there is any production BaNaNaS entry now that exceeds packet size. I believe --validate does this, but I am not really sure, I have to admit. Just something we need to take care of before merging :)

Copy link
Author

Choose a reason for hiding this comment

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

I agree, is this something you could try running in the server? or I can do somehow? since I don't have the files it only loads 4 of them (the ones you sent :-))

Copy link
Member

Choose a reason for hiding this comment

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

Tnx for the reminder, I forgot; all current content checks out fine, so that is good :)

That does remind me, we also need to change this in bananas-api:
https://github.com/OpenTTD/bananas-api/blob/7e9f4cf8c79cd6954607c8e43595d3c4d27e516e/bananas_api/new_upload/session_validation.py#L98

It ensures nobody can accidentally upload a blob bigger than what is allowed :)

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good! I'll propose those changes as well, keep u posted

Copy link
Author

Choose a reason for hiding this comment

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

There you go. OpenTTD/bananas-api#85

@LordAro
Copy link
Member

LordAro commented Mar 28, 2021

Given this is (primarily) meant to be read by the OTTD client, could simplify parsing stuff at the other end to just make it a unix timestamp, rather than a pre-formatted date? Adding a full ISO8601 parser to OTTD is non-trivial...

@TrueBrain
Copy link
Member

TrueBrain commented Mar 28, 2021

For master server protocol we use days since year 0, for which OpenTTD has code to make that into readable dates. We have no Python equivalent of it as far as I know, but shouldn't be hard to implement.

Might be an alternative?

@perezdidac
Copy link
Author

Thanks for the comments you all!
Changes made, upload date now getting passed as a 4 bytes UNIX epoch timestamp.

@TrueBrain
Copy link
Member

My only issue with 4byte unix timestamp is that it expires in 2038. You could argue how likely it is OpenTTD still exists then, but give we are midway between when OpenTTD was created (2004) and 2038, it is something to consider. One could hope we rewrote the protocol by then, or fix it when the date approaches, but looking at our 17 year history, not sure that is likely :p

Software is slowly moving to 8byte unix timestamps, but no clue if that is word the effort. This is btw why I mentioned days since year 0, as we know that works a lot longer :D hell, we could even fit that in a 16bit value if we offset it to start-year-of-bananas (2007). Just no clue if that is a good idea or not :D most likely it is not :p

I am +0 on this, but I did want to mention it so at least it is considered :) I have no problem making this a problem for future-us, honestly :D

@perezdidac
Copy link
Author

My only issue with 4byte unix timestamp is that it expires in 2038. You could argue how likely it is OpenTTD still exists then, but give we are midway between when OpenTTD was created (2004) and 2038, it is something to consider. One could hope we rewrote the protocol by then, or fix it when the date approaches, but looking at our 17 year history, not sure that is likely :p

Software is slowly moving to 8byte unix timestamps, but no clue if that is word the effort. This is btw why I mentioned days since year 0, as we know that works a lot longer :D hell, we could even fit that in a 16bit value if we offset it to start-year-of-bananas (2007). Just no clue if that is a good idea or not :D most likely it is not :p

I am +0 on this, but I did want to mention it so at least it is considered :) I have no problem making this a problem for future-us, honestly :D

First of all, OpenTTD WILL exist in 2038, I love making that assumption :)
BUT I am sure the BaNaNas API will be on a structured data messaging format like JSON or proto or obviously something more modern (it's 2038... maybe telepathy?). So yeah, I agree that we could assume we will have a better protocol by then. If not, then I personally commit to address this issue by then. Or maybe my kid does it if they are ever into OpenTTD (I'll try to encourage but not too hard).

I am OK going with either solution, the thing is that my PRs are normally controversial in a sense that I normally get mixed opinions from developers on how to do things, so I want to stick to this approach for now unless I get lots of push back, since I am afraid that if I end up passing it differently someone else might be completely opposed. What do you think? Again, I am happy with anything, I just want to get the feature in in a minimal clean way like this one ensuring long term and not breaking anything. Happy to chat over IRC or Discord live if you want, I am on PST.

And again, THANK YOU for your help and suggestions :)

perezdidac added a commit to perezdidac/bananas-api that referenced this pull request Apr 5, 2021
Adding the upload-date size (4 bytes) to the packet size validation that makes sure packets don't exceed OpenTTD packet size.

This change pairs up with OpenTTD/bananas-server#43 and OpenTTD/OpenTTD#8902 which aim to pass the content upload-date to OpenTTD so it can be used to display it and for being able to sort by upload date in the network content window.
perezdidac added a commit to perezdidac/bananas-api that referenced this pull request Apr 5, 2021
Adding the upload-date size (4 bytes) to the packet size validation that makes sure packets don't exceed OpenTTD packet size.

This change pairs up with OpenTTD/bananas-server#43 and OpenTTD/OpenTTD#8902 which aim to pass the content upload-date to OpenTTD so it can be used to display it and for being able to sort by upload date in the network content window.
@LordAro
Copy link
Member

LordAro commented Apr 6, 2021

I realise we're planning on replacing the network protocol in the "near future", but I'd still feel more comfortable if this were 8 bytes rather than 4. No need to be introducing more Y2k38 issues at this point :)

@perezdidac
Copy link
Author

I realise we're planning on replacing the network protocol in the "near future", but I'd still feel more comfortable if this were 8 bytes rather than 4. No need to be introducing more Y2k38 issues at this point :)

Thanks for the comment. @TrueBrain didn't seem to have a strong opinion on 4 vs 8 bytes, and I don't have it either. I gotta say in 2038 we'll definitely have a new network protocol, it seems to me there is no question here. I haven't explored how to pass a 8 bytes timestamp, doing a quick Google search I couldn't find much.

I see this feature as a temporary approach to pass upload-date until the new network protocol is done. In fact, I am interested in how the new network protocol would be interested as I've worked on many projects like this before and distributed systems, I'd love to help.

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

3 participants