pc2ccs / pc2v9

Version 9 of the PC^2 Programming Contest Control System
Eclipse Public License 2.0
46 stars 23 forks source link

Improve error handling/reporting/logging of errors/info in Shadow module #717

Open lane55 opened 1 year ago

lane55 commented 1 year ago

Is your feature request related to a problem?

Yes, there may not be enough logging in the Shadow module.

Feature Description:

Review Shadow code and add logging if missing.

Have you considered other ways to accomplish the same thing?

Not possible

Do you have any specific suggestions for how your feature would be **implemented in PC^2?**

Additional context:

lane55 commented 1 year ago

There are five conditions that Start Shadowing will fail on and show the message to the user "Shadow Mode not enabled, or shadowing parameters not valid", "Cannot start Shadowing" None of these conditions are logged. Best practice is to display an precise message like "Missing required password" for each condition.

lane55 commented 1 year ago

Throws exception on error during submitting a run, but silently done to log. For any error should do more than just log it. As a fill gap, write error with context to stderr, provide event line where error heard.
Best fix is to show the number of errors on the same page as the

|java.security.InvalidParameterException: Parameter clientIdString does not match any pc2 account: 'domjudge' | at edu.csus.ecs.pc2.shadow.RemoteRunSubmitter.submitRun (RemoteRunSubmitter.java:81) | at edu.csus.ecs.pc2.shadow.RemoteEventFeedMonitor.run (RemoteEventFeedMonitor.java:490) | at java.lang.Thread.run (Thread.java:745

lane55 commented 1 year ago

If there is an error opening the event feed, silently fails. Should provide a message that states the http error Also when an error, continues to loop with the same/following exception.

Opening connection to remote event feed starting at token 1 java.io.IOException: Server returned HTTP response code: 400 for URL: https://judge.gehack.nl/api/contests/bapc2022/event-feed?since_token=1 at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1876) at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1474) at sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:254) at edu.csus.ecs.pc2.shadow.RemoteContestAPIAdapter.getHTTPInputStream(RemoteContestAPIAdapter.java:149) at edu.csus.ecs.pc2.shadow.RemoteContestAPIAdapter.getRemoteEventFeedInputStream(RemoteContestAPIAdapter.java:225) at edu.csus.ecs.pc2.shadow.RemoteEventFeedMonitor.run(RemoteEventFeedMonitor.java:175) at java.lang.Thread.run(Thread.java:745)

btw in a browser https://judge.gehack.nl/api/contests/bapc2022/event-feed?since_token=1 returns: Invalid parameter "since_token" requested.

johnbrvc commented 1 year ago

As discussed at the PC2 Team meeting on March 14, 2023, some of the items mentioned above are covered under PR #575. However, there should be more detail supplied in the messages logged to the shadow notification JTable, in addition to handling some error conditions that current go unnoticed (can't fetch source, for example). (Note that ALL exceptions and errors are logged, but many do not have any GUI indication to the user that something went wrong).

Analysis of RemoteContestAPIAdapter reveals there are some exceptions that are caught there, but nothing is ever done with th em besides logging them (there is no indication on the GUI that something went wrong). There needs to be a way to pass these error conditions up to the RemoveEventFeedMonitor. This can be done in a couple of ways, both requiring changes to the IRemoteContestAPIAdapter interface. Changing this interface has relatively low impact on the system since not too many modules actually use it besides Shadow (the only other one ATM is ContestCompareModel for report generation). One way to give allow access to the errors occurring in the RemoteContestAPIAdapter is to just allow any exceptions to be thrown back up to the next layer. Another is to do it the Windows and Unix way: provide a "GetLastError()" type call (eg "errno" on Unix) so when an call fails, the caller can then call the GetLastError() to find out the details of the error (perhaps return a string describing the last error).

As for being unable to fetch the source code for a submission, this can be done without changing the IRemoteContestApiAdapter interface, since it is handled in RemoteEventFeedMonitor.

My suggestion is a 2 phase approach: 1) go through RemoteEventFeedMonitor and any error that is logged but not reported to the GUI should be reported using the IShadowMonitorStatus interface so it appears in the ShadowControlPane notification table. This should be a low LOE. 2) determine the best way to pass errors from the RemoteContestAPIAdapter back to the RemoteEventFeedMonitor (or any other module using it). Choice 1 would be using exception handlers, Choice 2 would be a GetLastError() type mechanism. Choice 1 requires a higher LOE than Choice 2. I think Choice 2 was what the original intention was when the code was written - don't require the caller to have to worry about every exception than can occur. Just return and error (null) if something goes wrong, and leave it up to the caller to figure out what to do.

Since I am pretty familiar with this code at this point, I would be more than happy to implement whatever we decide to do.

lane55 commented 1 year ago

Changes added to add Exceptions in Shadow to test connetion list box in #758.