search

CreeperHost / modpacklauncher

Other
22 stars 3 forks source link

Error Handling/Logging Suggestions #3

Open modmuss50 opened 4 years ago

modmuss50 commented 4 years ago

This issue just contains my ideas on how better error handling or logging should look, feel free to suggest you own ideas as what I suggest here may not be what you are after at all.

The modpack installation (the only bit of the code ive really looked at) seems to have limited error handling, including ingoring what should be fatal expetions.

Logging

  1. I would suggest using a logging library such as log4j, with a good config file it will make it easy to save logs to disk. As this is a launcher/wrapper for minecraft using a similar file structure to the game may make it more familar to users.

  2. On this, the logger instance should be created per class, this will make it easy to see where a log message is coming from. The existing CreeperLogger is very basic and should be replaced with a fully fleshed out logging api.

Expectation's

There are many cases in the codebase where expections are caught, but not handled, this causes stages further down the installation chain to contiune even when a prior task has failed. To slove this I would suggest creating some custom expections that can be chained to make errors easier to debug, and easier to handle.

For example ModLoaderInstallationExpectation could be thrown anytime something goes wrong when installing a modpack. For example here in ForgeJarModLoader.java. ModLoader.install would throw this error.

This expeption would then be handled here in FTBModPackInstallerTask.java and could then throw a ModPackInstallationExpectation. This would then be passed further up the chain and handled when the button is pressed, a useful expetion message could be shown to the user and a full detailed stackstrace saved to the logs.

If a ModPackInstallationExpectation was thrown the installer should ensure that the instantace is not left in a courpt state, either by reverting to a previous version, or removing the instance.

Other notes

CompletableFuture seems to only be used to run code async without any return values, CompletableFuture's are great for chaining stages togeather and also pass on the expetions correctally. Moving things over to use more of CompletableFuture's features might require quite a bit of work all over the codebase, but would eventually lead to being able to progress at a fast rate.

I think the best way forward is to not cause drastic changes as it could easily cause unwanted bugs, and slow down updates for the users. Chipping away slowing refactoring and improving certain parts of the codebase over time seems like a logical way to go about this.

I created this issue based off what was said in the #ftb-app discord channel on request of Jake. Feel free to provide your own solution as this is just the way that I would go about doing this.

This is in by no means trying to bash the existing code, as it has proven it works well. It is about ensuring that this codebase can be maintained and expanded for years to come without needing to do larger refactors down the line.

ThePaul-T commented 4 years ago

I saw your discord chat earlier and went through the code as I was concerned on the comments. (For example, URI and URL are used because of the possibility of raw URL encoding and URL encoding on links, of which java appears to only handle "raw" URL encoding(%20) with URI's and URL encoding(+) with URL's so to get a fully usable object, it needed to traverse both - Not a problem if we weren't handling user-submitted data) - There may be better ways to handle this case, it may have been caused by a different bug, but it was an issue.

I can confirm if any files fail the sha1 checksumming an exception is thrown and that is then passed back over WebSocket to the frontend, so it should not be possible for a corrupted modpack. (You'll see my custom exception in there, it's terrible), we leave the UX of that (a model telling them it failed or similar with the option to delete or retry) to the frontend implementation.

We only warn on filesize at the moment because there are a number of factors (gzip, lzma etc) which can cause false positives here, and it was not worth the hassle it caused (happened a fair bit in development while we tried different webserver setups)

I can't vouch for any of the previous or current mod-loader (download or otherwise) implementation as that was not my work, so I'll probably take a look in the near future.

My work was mostly LocalInstance, LocalCache (both my design) and then shoehorning some of the actual processes into other classes - Be easy on my work please, I actually "learnt" (what little I've managed thus far) java as I was working.

Logging was not mine but I agree completely with it being sub-par and poorly formatted.

Finally, I'd like to thank you for actually trying to lend a hand, providing actual feedback in a constructive way.

Note: We've not actually touched this codebase in a meaningful way for 3+ months, we've been focused on the front-end and installer. So it is not finished in the slightest.