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

Fix if spoolman spool is missing #791

Closed
wants to merge 1 commit into from
Closed

Conversation

Zeanon
Copy link

@Zeanon Zeanon commented Jan 15, 2024

Currently if the last selected spool gets deleted from the spoolman database, the spoolman component fails to load. This fix sets the selected spool to None if that happens so the component can load

Currently if the last selected spool gets deleted from the spoolman database, the spoolman component fails to load.
This fix sets the selected spool to None if that happens so the component can load
@Arksine
Copy link
Owner

Arksine commented Jan 16, 2024

Hi. A 404 returned by the proxy request would not cause a failure to load the Spoolman component. The front end is misreporting it as such because it believes that the proxy endpoint doesn't exist due to the 404, when its Spoolman is returning 404. While this is understandable, I think that frontends need to treat errors as if they were returned by Spoolman when using this endpoint. Moonraker reports a list of loaded components and failed components in its /server/info endpoint.

I don't think its a good idea to change the active spool in the proxy endpoint. The message string isn't a reliable source for comparison, if the error response changes just one character we will always evaluate to False. In addition, having Moonraker return the message in place of a 404 error represents an API change that some clients may not be able to deal with.

There may be some things we can do better to help alleviate this issue, but regardless of what we do changes will need to be made at the frontend to accommodate it.

@Zeanon
Copy link
Author

Zeanon commented Jan 16, 2024

Fair enough, it was just a initial draft, I'm open to new ideas
I figured the safest way to only detect missing spools was, to compare the error message(as that indicates that exact error)
But out of curiosity: the component failed to load message you get in mainsail, is that from mainsail or moonraker?
cause if it's moonraker, then moonraker does need to be changed as imo, it should not prevent the component from loading of the spool that was registered last got deleted(especiall since you have no way of changing it when you can't access the component)

Might also be a good idea to talk to the guy from spoolman and maybe add a specifiy error code for spool not found?

@Arksine
Copy link
Owner

Arksine commented Jan 19, 2024

I just opened #792 with the aim of resolving this issue. I makes some changes that should reduce the chance that Moonraker's spool id does not exist in the Spoolman Database. In addition it makes an API change that will allow frontends to deal with the ambiguous 404 error, however it will require implementation on their side.

But out of curiosity: the component failed to load message you get in mainsail, is that from mainsail or moonraker?

Based on the behavior described, I'm reasonably certain that the component hasn't actually failed. The ambiguous 404 returned when a spool doesn't exist translates to Method not found, so Mainsail thinks that the component isn't loaded when it is.

Might also be a good idea to talk to the guy from spoolman and maybe add a specifiy error code for spool not found?

In this case 404 is the right code to return. I think correcting this issue in Moonraker and the frontends is the appropriate solution.

@Zeanon
Copy link
Author

Zeanon commented Jan 19, 2024

I just opened #792 with the aim of resolving this issue. I makes some changes that should reduce the chance that Moonraker's spool id does not exist in the Spoolman Database. In addition it makes an API change that will allow frontends to deal with the ambiguous 404 error, however it will require implementation on their side.

Problem is that moonraker can't control whether the spool gets deleted.
Initially the problem occured cause I created a dummy spool to test spoolman (cause I am running it on a remote server with an automated ssh tunnel so the printers can access it and so on, not important here), tldr I wanted to test stuff and after testing I delketed the dummy spool, cause why keep it, it was just a dummy for testing.
Then spoolman stopped working on 2 of 3 printers while I could still access the url via terminal, so I was quite confused.

Concerning the mainsail reads error wrong, I'll try to come up with a patch for that and do a pr on mainsail, I just figured it would be a moonraker error, since I got a moonraker warning(which is actually mainsail, which is confusing XD)

And concerning the error code: I am not that much of a web dev and not that much into error codes, so my bad I guess :)
I just figured opening a pr and presenting a solution is better than just complaining :)

@Arksine
Copy link
Owner

Arksine commented Jan 23, 2024

This should now be resolved on Moonraker's side. The major known frontends have changes complete or in process.

@Arksine Arksine closed this Jan 23, 2024
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.

2 participants