Community
Participate
Working Groups
When an error occurs when connecting to a server, downloading a file, etc. the error message is sometimes cryptic and it would help to have it more specific (e.g. missing file vs unknown host, etc).
Seems like this is a duplicate of bug 258896. If this is about something else, please provide more detail.
*** Bug 258896 has been marked as a duplicate of this bug. ***
We got the duplicate resolved the other way around. Ah well, then this is the issue for improving the error messages. Copying the text from bug 358896 When an error occurs downloading something through our ECFTransport layer, the message invariably comes back saying something like: "Can't find the artifact XXX". This is a good start but in comparison to many other software doing similar things the message is generally more detailed root cause: - file does not exist - host does not exist. - time out. - ... See also bug #226769 as this might be helpful in implementing improvements for this bug.
I picked this bug because it is in the Equinox plan: http://www.eclipse.org/projects/project-plan.php?projectid=rt.equinox Also, we generally make newer bugs a duplicate of older bugs unless the newer one has a much better description, patch, etc.
(In reply to comment #4) > I picked this bug because it is in the Equinox plan: > > http://www.eclipse.org/projects/project-plan.php?projectid=rt.equinox > > Also, we generally make newer bugs a duplicate of older bugs unless the newer > one has a much better description, patch, etc. > Thanks for the clarification. I will remember that.
I have been looking into the code in ECFTransport, and I have a hard time figuring out where details are being lost - it seems like a reasonable job is done to include caught exceptions in the status that is ultimately returned. Any input on what I should be looking for? Things known to be in need of a rewrite would be really helpful. Also, if you have some hints on what to test this against (to trigger various problems). Seems like a set of repositories broken in different ways would be a useful thing to have. I found one peculiarity - this is done in transfer: } catch (IncomingFileTransferException e) { if (e.getErrorCode() == 401) throw ERROR_401; //try to figure out if we have a 401 by parsing the exception message IStatus status = e.getStatus(); Throwable exception = status.getException(); if (exception instanceof IOException) { if (exception.getMessage() != null && (exception.getMessage().indexOf(" 401 ") != -1 || exception.getMessage().indexOf(SERVER_REDIRECT) != -1)) //$NON-NLS-1$ throw ERROR_401; } return statusOn(target, status); Is it really necessary to check again for an IOException with a message text including "401"? I think that the IncomingFileTransferException did not use to have a getErrorCode() - and I wonder if this is old remains, or really needed because not all cases of a 401 is reported as such via the error code...
Created attachment 126668 [details] Screenshot of the problematic dialog I believe that the problem lies at the intersection of ECF and p2 where ECF does not return a status code that is precise enough to allow for a presentation of a nice message. (Indeed here, given that ECF is abstracting from callers the details about transport when getting a file, I would argue that to be consistent it should do the same for error code) For example I have tried to add a repo with a bogus host and I get the dialog attached saying "transfer exception". This is not useful and it is the kind of things that I'm talking about. For ideas of what we should get at, try to think about everything that goes wrong when you type in a URL in your browser or when something goes wrong while you are obtaining a page.
Some interesting runtime exceptions are thrown on bogus input. Reported bug 266140 where a NumberFormatException is thrown when the url is "http:bogus". (Fixing it temporarily by catching NumberFormatException and taking this to mean "malformed remote file reference".
Created attachment 126801 [details] ECFTransport with error handling + test The patch contains changes to ECFTransport, and additional helper class where (almost) all the Exception and Status translation takes place. A test class is also included (intended to be run interactively to monitor the output of error messages - can be augmented with more URLs to trigger additional errors). The implementation in ECFTransport also fixes/works around these issues: - Hanging on wait if there is a connection error (UnknownHost, and similar problems) - a listener was added to the connect job to enable proper notification. - Faulty input such as http:bogus is handled (ECF throws a NumberFormatException) - see bug 266140 (The implementation can remove this restriction). - "non URI" compliant input like "g!arbage!!" caused IllegalArgumentExceptions - this is now caught.
The handling of 401 by parsing the exception message is clearly not optimal, but that was left there while we phased in the ECF change that propagated error codes. For example at one point it worked ok with Apache HTTP client, but not with the default JRE HTTP client. If we have done enough testing to be confident the error code alone is sufficient, we could pull out the code that parses the exception message. Maybe we can start by logging a debug message (only in debug mode) when the message contains 401 but the error code is not 401, to see if this still occurs.
(In reply to comment #7) > Created an attachment (id=126668) [details] > Screenshot of the problematic dialog I noticed that the dialog does not show the type of exception which it could. I think at least for some portion of the users it would make the message more meaningful if it would say UnknownHostException: sdkjaljlf.com > > I believe that the problem lies at the intersection of ECF and p2 where ECF > does not return a status code that is precise enough to allow for a > presentation of a nice message. (Indeed here, given that ECF is abstracting > from callers the details about transport when getting a file, I would argue > that to be consistent it should do the same for error code) > > For example I have tried to add a repo with a bogus host and I get the dialog > attached saying "transfer exception". This is not useful and it is the kind of > things that I'm talking about. > For ideas of what we should get at, try to think about everything that goes > wrong when you type in a URL in your browser or when something goes wrong while > you are obtaining a page. > HTTP (for example) typically gives you a status line that has a short message. In addition the retrieved html page will have similar info in more detail and potentially in the users locale. Is this the type of information that should be presented?
(In reply to comment #11) > I noticed that the dialog does not show the type of exception which it could. I > think at least for some portion of the users it would make the message more > meaningful if it would say > UnknownHostException: sdkjaljlf.com > That is what it should say when applying the patch in this issue. Then there may be issues with the dialog and how it presents the error. In the patch I have only worked on getting more detailed messages instead of "something bad happened". > HTTP (for example) typically gives you a status line that has a short message. > In addition the retrieved html page will have similar info in more detail and > potentially in the users locale. > Is this the type of information that should be presented? > The patch contains error messages for all of the HTTP 400 and 500 error codes and a short text is presented if the code is set in the exception. There is special handing for common cases that falls outside of the "HTTP return code". The patch also organizes the error handling in a way that makes it easier to add more details/change individual messages as they are detected in real life use, and by using different protocols. I think the patch is the first step in having better error messages. But as it is difficult to generate all of the potential errors, I am sure there will be things discovered that should be changed.
Hm, ECFMetaDataTransport also needs to be impoved, and there has been some recent changes to how ECF handles some of the exceptions and ECFTransport is changed. Will work on a new patch.
Status update. This turned out to be more work as there are three different implementations of transports that deal with exceptions from ECF. These are in the process of being merged - see bug 216278 which will also include better error handling.
See bug #267747 for a description of the policy for exceptions in done event.
Can this be marked fixed? I see there are now custom error messages for the different error codes. And good job on handling error code 418 - I just noticed this today, which was perfect timing ;)
(In reply to comment #16) > Can this be marked fixed? I see there are now custom error messages for the > different error codes. > Yes, this can be closed. Maybe open a new issue that tests are needed. There are error codes for all relevant HTTP codes, but it is not well tested what actually happens. > And good job on handling error code 418 - I just noticed this today, which was > perfect timing ;) > Did you actually see that error and the message?
> Did you actually see that error and the message? No, I didn't find any web-enabled tea pots to test this against. I just noticed it while reviewing incoming changes. I think support for robust coffee provisioning will have to wait until the next release...
