Posted July 16, 2018 com.wurmonline.server.intra.PlayerTransfer public static final void sendItem(...) .... try { if (item.getTemplateId() == 1310) { dos.writeBoolean(true); final long animalId = item.getData(); final Creature animal = Creatures.getInstance().getCreature(animalId); CreatureDataStream.toStream(animal, dos); if (animal.getDominator() != null) { for (final Player player : Players.getInstance().getPlayers()) { if (player.getPet() == animal) { player.setPet(-10L); } } animal.setDominator(-10L); } } else { dos.writeBoolean(false); } } catch (Exception e) { e.printStackTrace(); } com.wurmonline.server.intra.IntraServerConnection public static void createItem(...) try { final boolean isStoredAnimalItem = dis.readBoolean(); if (isStoredAnimalItem) { CreatureDataStream.fromStream(dis); } } catch (IOException e) { e.printStackTrace(); } Share this post Link to post Share on other sites
Posted July 16, 2018 Did your post get cut off? I assumed you'd have an example of something better. Share this post Link to post Share on other sites
Posted July 16, 2018 I think his point is that the exception should be at least logged, not just dumped into stderr. Share this post Link to post Share on other sites
Posted July 16, 2018 3 minutes ago, bdew said: I think his point is that the exception should be at least logged, not just dumped into stderr. Yeah, we kind of figured that's what he meant. I just wished he used his words. Share this post Link to post Share on other sites
Posted July 16, 2018 (edited) 13 minutes ago, Keenan said: Yeah, we kind of figured that's what he meant. I just wished he used his words. Sorry but that are bad practises in java coding, there shouldn't be much of an explanation needed. This bad practice is casuing this horrible bug: So it is not just some theoretic thing I tell you because I am bored. My time is precious and expensive. Edited July 16, 2018 by Sklo:D Share this post Link to post Share on other sites
Posted July 16, 2018 3 minutes ago, Sklo:D said: Sorry but that are bad practises in java coding, there shouldn't be much of an explanation needed. This bad practice is casuing this horrible bug: So it is not just some theoretic thing I tell you because I am bored. My time is precious and expensive. We've not had any players getting deleted since this update on WO. I've not had any complaints outside of the one you've posted for WU. As for what it should be doing: I'd prefer crossing to be a transaction that can be rolled back, but we're not there yet. Instead it should be logged as bdew said above. This we are aware of. However your "precious time" is not a license to snub or be uncivil with us and if you are unable to report issues in a civil way then I'd much prefer you not report them at all. Share this post Link to post Share on other sites
Posted July 16, 2018 (edited) 15 minutes ago, Keenan said: We've not had any players getting deleted since this update on WO. I've not had any complaints outside of the one you've posted for WU. As for what it should be doing: I'd prefer crossing to be a transaction that can be rolled back, but we're not there yet. Instead it should be logged as bdew said above. This we are aware of. However your "precious time" is not a license to snub or be uncivil with us and if you are unable to report issues in a civil way then I'd much prefer you not report them at all. Not yet because there is another bug sliding in. Go on Xanadu and type #checkCreatures in chat and you will have many many many people having problems. Nobody would get deleted because you need a creature cage in the inventory for that, (we had a GM deleted) but things would get quite interesting on server transfers with ships holding creature cages And that is why it is a horrible idea to handle exceptions with the method of "I have no time and don't care about exceptions because I don't care about code quality". Things will always go wrong, if there is no creature exisitng but a creature transport item with the id of a non existent creature then there will be a TRUE for the is creature item part but there will be no creature stream because the method ran into a catch and is not handled correctly. At some point there will be an EOFException because obviously parts are missing. Edited July 16, 2018 by Sklo:D Share this post Link to post Share on other sites
Posted July 16, 2018 Without diving through the code, what you're saying is lagging the server during transfer causes this? I'm not sure if that's the case entirely. I know severe lag doesn't play well with transfers, but that's not the thing that would be shown in those stack traces. I do see a few things that I've mentioned to the original developer of this feature though. For one, when working within the Wurm code base, it's better to be safe than sorry when it comes to objects. Null check the living snot out of it. If it is a problem in CreatureDataStream, then I'd say it's more likely an object on animal is a null reference. And in that case, you're right - the stderr printing of the stack is probably hiding the culprit. Share this post Link to post Share on other sites
Posted July 16, 2018 (edited) 6 minutes ago, Keenan said: Without diving through the code, what you're saying is lagging the server during transfer causes this? I'm not sure if that's the case entirely. I know severe lag doesn't play well with transfers, but that's not the thing that would be shown in those stack traces. I do see a few things that I've mentioned to the original developer of this feature though. For one, when working within the Wurm code base, it's better to be safe than sorry when it comes to objects. Null check the living snot out of it. If it is a problem in CreatureDataStream, then I'd say it's more likely an object on animal is a null reference. And in that case, you're right - the stderr printing of the stack is probably hiding the culprit. #checkCreatures DELETES ALL creatures in creature cages on the server but doesn't touch the id stored in the cages. So we will find oursleves in the mentioned catch block of sendItem(..) and trigger an EOFException on the login server for every creature cage which has a creature in it before the command was entered. Edited July 16, 2018 by Sklo:D Share this post Link to post Share on other sites
Posted July 16, 2018 3 minutes ago, Sklo:D said: #checkCreatures DELETES ALL creatures in creature cages on the server but doesn't touch the cages. Welp, that functionality changed a bit. Or is a bug. I'll take a look. We rarely use it on WO, mostly because of the lag portion of it. Share this post Link to post Share on other sites
Posted July 16, 2018 (edited) 6 minutes ago, Keenan said: Welp, that functionality changed a bit. Or is a bug. I'll take a look. We rarely use it on WO, mostly because of the lag portion of it. We liked to use it because it keeps the server clean and worked good. But I am sure there are other possibilities for having creatures going missing in creature cages too. Over time there can be a lot strange things happening, so just fixing the command alone is not a good solution, there should be a fix for transfering creatures in general to prevent this in the future. Edited July 16, 2018 by Sklo:D Share this post Link to post Share on other sites
Posted July 16, 2018 3 minutes ago, Sklo:D said: We liked to use it because it keeps the server clean and worked good. But I am sure there are other possibilities for having creatures going missing in creature cages too. Over time there can be a lot strange things happening, so just fixing the command is not a good solution, there should be a fix for transfering creatures in general to prevent this in the future. Yeah, I've already stated that we should review that code and decide upon a course of action if the animal has errors. My non-caffeinated review of it sees some potential NPEs with bad data as I mentioned above. Share this post Link to post Share on other sites
Posted July 16, 2018 (edited) 8 minutes ago, Keenan said: Yeah, I've already stated that we should review that code and decide upon a course of action if the animal has errors. My non-caffeinated review of it sees some potential NPEs with bad data as I mentioned above. This is how you can improve the code to be a whole lot more stronger against errors in under 5 minutes. Tell your devs to take the time, it is worth it in the end. try { if (item.getTemplateId() == 1310) { final long animalId = item.getData(); final Creature animal = Creatures.getInstance().getCreature(animalId); dos.writeBoolean(true); //do this when you are sure that the creature exists CreatureDataStream.toStream(animal, dos); if (animal.getDominator() != null) { for (final Player player : Players.getInstance().getPlayers()) { if (player.getPet() == animal) { player.setPet(-10L); } } animal.setDominator(-10L); } } else { dos.writeBoolean(false); } } catch (NoSuchCreatureException e) { log.warning(..); dos.writeBoolean(false); //remove the creature id from the cage??? } catch (IOException e) { log.error(....); //abort transfer of this item?? } Edited July 16, 2018 by Sklo:D Share this post Link to post Share on other sites
Posted July 16, 2018 Well I'd add a bit more to that. It's not just existing - since the NSCE should prevent a non-existent animal from being added. It's also making sure the animal is complete and that we can send all the data points. I had a similar thought to yours, but yours is also incomplete. Basically because we're writing to the stream directly, we need to make sure everything is clean up front. Share this post Link to post Share on other sites
Posted July 16, 2018 4 minutes ago, Keenan said: Well I'd add a bit more to that. It's not just existing - since the NSCE should prevent a non-existent animal from being added. It's also making sure the animal is complete and that we can send all the data points. I had a similar thought to yours, but yours is also incomplete. Basically because we're writing to the stream directly, we need to make sure everything is clean up front. I agree the whole thing could get some more error handling care in general. I just wanted to point out how you can catch 90% of all errors in under 5 minutes of work. Of course the last 10% are a bit more tricky to handle. Share this post Link to post Share on other sites
Posted July 16, 2018 What I really want to do, and time isn't being kind to me: Create DTOs for each entity being sent over the stream. Serialize DTOs to json and send json over the stream as bytes. The DTOs can nest everything nicely if the serialization library is worth its salt. It also would allow us to catch bad data before sending it and validate the sent data before converting it back to the entities. This could be applied to the client as well and even expanded upon by including the protocol version. That way a strategy pattern of sorts could be used to handle messages from clients of older protocols rather than our methodology now. Ideally, we shouldn't have to care about protocols on WO since people should be using the most up-to-date client, but we are aware of ways to defeat that (simply by opening the client quickly and not waiting!). WU presents an even better argument for this though, as it would be a decent start for allowing better previous version compatibility with newer servers. Anywho, I really just wanted to put this somewhere because it's been in my head for far too long. I've got a lot on my plate, but this would be a nice future revision (that would break all mods that send data, but in a good way!). I mean speaking on that front, I could easily add a way for a mod to hook into the stream and send its own json and the client can be smart enough to process what it knows and discard what it doesn't. That'd open the doors quite a bit. Share this post Link to post Share on other sites
Posted July 16, 2018 (edited) 57 minutes ago, Keenan said: What I really want to do, and time isn't being kind to me: Create DTOs for each entity being sent over the stream. Serialize DTOs to json and send json over the stream as bytes. The DTOs can nest everything nicely if the serialization library is worth its salt. It also would allow us to catch bad data before sending it and validate the sent data before converting it back to the entities. This could be applied to the client as well and even expanded upon by including the protocol version. That way a strategy pattern of sorts could be used to handle messages from clients of older protocols rather than our methodology now. Ideally, we shouldn't have to care about protocols on WO since people should be using the most up-to-date client, but we are aware of ways to defeat that (simply by opening the client quickly and not waiting!). WU presents an even better argument for this though, as it would be a decent start for allowing better previous version compatibility with newer servers. Anywho, I really just wanted to put this somewhere because it's been in my head for far too long. I've got a lot on my plate, but this would be a nice future revision (that would break all mods that send data, but in a good way!). I mean speaking on that front, I could easily add a way for a mod to hook into the stream and send its own json and the client can be smart enough to process what it knows and discard what it doesn't. That'd open the doors quite a bit. A strategy pattern is the best thing to achieve backwards compatibility for sure. I personally wouldn't use JSON for object serialization when Java has such a great inbuilt funcitonality for serialization, we have been using serialized objects a lot and never had any problems with that functionality. Why should we use JSON, when java provides a fully secured serialization API. But also the plan with the DTOs sounds great, Wurm needs more DTOs so much things are passed by hand, which makes code super ugly and hard to maintain. Simple example for serialization: https://www.journaldev.com/927/objectoutputstream-java-write-object-file Edited July 16, 2018 by Sklo:D Share this post Link to post Share on other sites
Posted July 16, 2018 JSON comes from my day job mostly. I'm used to developing things that must be easily consumed by a myriad of other platforms. I'll take a look at the built-in serialization. At the end of the day, JSON is just a format. The real key is to actually use serialization while keeping that layer we need to be able to make opinions about what gets serialized and what doesn't. Share this post Link to post Share on other sites
Posted July 16, 2018 8 minutes ago, Keenan said: JSON comes from my day job mostly. I'm used to developing things that must be easily consumed by a myriad of other platforms. I'll take a look at the built-in serialization. At the end of the day, JSON is just a format. The real key is to actually use serialization while keeping that layer we need to be able to make opinions about what gets serialized and what doesn't. Things you don't want to serialize can just be made transient: eg.: private transient long unimportant; so java is quite powerful there. I also like JSON, I just expect building a JSON serializer to be more complex than taking the inbuilt one. I am using JSON a lot with REST. Share this post Link to post Share on other sites
Posted July 16, 2018 1 minute ago, Sklo:D said: Things you don't want to serialize can just be made transient: eg.: private transient long unimportant; so java is quite powerful there. I also like JSON, I just expect building a JSON serializer to be more complex than taking the inbuilt one. I am using JSON a lot with REST. Yeah I keep forgetting Java, unlike Golang, doesn't have native support for serializing to JSON. Though there are some good third party libraries out there I hear (like Gson). The same can be said for C#, where we use NewtonSoft at work since Microsoft's serialization to JSON is junk. (Why wouldn't you use RFC3999 for dates Microsoft? Huh? WHY...) I pretty much work with C#, Java, Golang, and PHP - as well as various bash scripts - in my day job to keep a legacy system working. We're slowly rewriting it, but I'm the last guy on the team for the moment. So when you say time is precious, really ... I get it. I totally do. Even right now I'm likely going to be working 10 minutes extra for every minute I spend typing a reply. Share this post Link to post Share on other sites
Posted July 18, 2018 (edited) On 7/16/2018 at 4:15 PM, Keenan said: Yeah I keep forgetting Java, unlike Golang, doesn't have native support for serializing to JSON. Though there are some good third party libraries out there I hear (like Gson). The same can be said for C#, where we use NewtonSoft at work since Microsoft's serialization to JSON is junk. (Why wouldn't you use RFC3999 for dates Microsoft? Huh? WHY...) I pretty much work with C#, Java, Golang, and PHP - as well as various bash scripts - in my day job to keep a legacy system working. We're slowly rewriting it, but I'm the last guy on the team for the moment. So when you say time is precious, really ... I get it. I totally do. Even right now I'm likely going to be working 10 minutes extra for every minute I spend typing a reply. My main languages I use for my software products are Java and PHP, but I also did a lot with tons of other langugages and I hate C#. Got my own small company running the sklotopolis server and also doing a lot android apps for example apps for gps live tracking. A good way for me to get some knowledge next studying software engineering at a technical university. Software Quality assurances failed at your company as it seems. Nothing better than an old software not documented, without layer seperation and no interface pattern used. Small fun fact: The server seems to have handled the incomplete packets and interpreted it: How I know that? 14816 and 14821 are ids of my servers and there is no effect with type 14816 or 14821. Christmas lights everywhere --- Merry Christmas to you! wurmitems.db --- table EFFECTS: "100" "100" "0" "14816" "0.0" "0.0" "0.0" "3554736552416" "115" "115" "0" "14816" "0.0" "0.0" "0.0" "72057594037927935" "213" "213" "0" "14816" "0.0" "0.0" "0.0" "144115188075855872" "220" "220" "0" "14816" "0.0" "0.0" "0.0" "3554736159200" "236" "236" "0" "14816" "0.0" "0.0" "0.0" "33269219340" "241" "241" "0" "14816" "0.0" "0.0" "0.0" "3554736159200" "257" "257" "0" "14816" "0.0" "0.0" "0.0" "15284043778" "314" "314" "0" "14816" "0.0" "0.0" "0.0" "144115209605000249" "379" "379" "0" "14816" "0.0" "0.0" "0.0" "673422522853" "446" "446" "0" "14816" "0.0" "0.0" "0.0" "2135564106213" "460" "460" "0" "14816" "0.0" "0.0" "0.0" "144115188075855872" "466" "466" "0" "14816" "0.0" "0.0" "0.0" "144115188078692018" "520" "520" "0" "14816" "0.0" "0.0" "0.0" "1666172271072" "535" "535" "0" "14816" "0.0" "0.0" "0.0" "54207184901" "575" "575" "0" "14816" "0.0" "0.0" "0.0" "144115188075855872" "642" "642" "0" "14816" "0.0" "0.0" "0.0" "3554736552416" "657" "657" "0" "14816" "0.0" "0.0" "0.0" "0" "681" "681" "0" "14816" "0.0" "0.0" "0.0" "3554736159200" "697" "697" "0" "14816" "0.0" "0.0" "0.0" "50717523968" "853" "853" "0" "14816" "0.0" "0.0" "0.0" "144115188075855872" "917" "917" "0" "14816" "0.0" "0.0" "0.0" "144115188077407948" "950" "950" "0" "14816" "0.0" "0.0" "0.0" "144115188075855872" "25" "25" "0" "14821" "0.0" "0.0" "0.0" "144115188078692502" "302" "302" "0" "14821" "0.0" "0.0" "0.0" "144115188078664894" "333" "333" "0" "14821" "0.0" "0.0" "0.0" "144115188075855872" "371" "371" "0" "14821" "0.0" "0.0" "0.0" "144115188075855872" "393" "393" "0" "14821" "0.0" "0.0" "0.0" "144115188075855872" "636" "636" "0" "14821" "0.0" "0.0" "0.0" "144115188075855872" "813" "813" "0" "14821" "0.0" "0.0" "0.0" "144115198221344057" "832" "832" "0" "14821" "0.0" "0.0" "0.0" "144115188075855872" Edited July 18, 2018 by Sklo:D Share this post Link to post Share on other sites
