Ticket #560 (assigned todo)

Opened 6 years ago

Last modified 3 years ago

When we can't find a library, we shouldn't silently continue and give weird errors later

Reported by: wayland Owned by: whiteknight
Priority: trivial Milestone:
Component: core Version: master
Severity: medium Keywords:
Cc: Language:
Patch status: Platform: all

Description

In particular, I'd argue that we should quit with an error, and show the path we searched.

Attachments

parrot_libloading_2.patch Download (3.4 KB) - added by wayland 6 years ago.
Code to show path during error while loading library
parrot_libloading_3.patch Download (2.9 KB) - added by wayland 6 years ago.
Updated patch with improvements
parrot_path_str.patch Download (2.1 KB) - added by wayland 5 years ago.

Change History

Changed 6 years ago by wayland

Code to show path during error while loading library

  Changed 6 years ago by wayland

Updated patch, thanks to changes suggested by bacek, and help from others.

  Changed 6 years ago by NotFound

The format %S is obsolete, you must use %Ss

Changed 6 years ago by wayland

Updated patch with improvements

follow-up: ↓ 4   Changed 6 years ago by wayland

Fixed what NotFound mentioned.

in reply to: ↑ 3   Changed 6 years ago by Infinoid

Thanks for the patches!

After discussing this with you on IRC and reading through some of the code, I'm trying to figure out whether this is the right approach. Some input from the parrot designers and/or HLL guys would help. Some more specifics on the motivation for this patch would also help; can you provide a test case, or details on how to reproduce your silent failure?

There's a comment in Parrot_load_lib:

        /*
         * XXX Parrot_ex_throw_from_c_args? return PMCNULL?
         * PMC Undef seems convenient, because it can be queried with get_bool()
         */

That seems to indicate a lack of clear design choice about how to report failure. If it's still an open question, I think throwing an exception here (as your patch does) might be the right choice, if only because this code has better knowledge of what the search path was, and thus it can generate a more useful error message than the callers might.

So, tentative +1 from me.

Mark

  Changed 6 years ago by allison

Thanks for the patch!

Agreed on throwing an exception. Exceptions used to be difficult to handle, so some of the core code avoided them. The direction we're headed now is towards exceptions for this kind of failure.

Since this is a fundamental change to the behavior of a core (and commonly used) opcode, it will need a full deprecation cycle. If a deprecation notice is entered now, the change could be made after the 1.4 release in July. That gives HLL developers a chance to try it out and see if it is likely to cause them any problems.

Agreed with Mark that it would be helpful if you'd provide more details on the "silent failure" you mention, and a test case or example code. This change is a good one to make, but there may be other aspects to the problem that this patch doesn't address.

And, while we wait on deprecation cycles, feedback from HLL developers, and more information on the failure, there's some work you can do on the patch. 'Parrot_path_str' needs a better name, to indicate that it's preparing a printable string of the current search paths. Something like 'Parrot_lib_search_paths_as_string'. ('lib' being the subsystem identifier required by the coding conventions.) This function can be added now, and doesn't have to wait until after 1.4, so submit it as a separate patch.

General stylistic/coding convention changes:

  • Don't include a comment to say that X or Y has changed, comments should describe the current state of the code. In this case, you can just delete the comment, which was a TODO.
  • Don't include your name in comments, though you can mention the patch in CREDITS if you want to.
  • Do update the documentation for the function when the behavior changes. In this case there is a mention of throwing an exception, so you just need to expand the cases listed.
  • Remove old code (the "return pmc_new" line) rather than commenting it out. In this case, I expect you left in the commented code to make it easier to review, so this is a "change before applying the patch".

Thanks! Allison

Changed 5 years ago by wayland

  Changed 5 years ago by wayland

Ok, I've attached the "separate patch" that Allison mentioned. Let me know if there are problems with it.

  Changed 5 years ago by jkeenan

  • component changed from none to core

  Changed 4 years ago by whiteknight

  • owner set to whiteknight
  • platform set to all
  • version set to master
  • type changed from bug to todo

  Changed 4 years ago by jkeenan

whiteknight,

Is the patch submitted by wayland still under consideration?

Thank you very much.

kid51

  Changed 4 years ago by whiteknight

  • status changed from new to assigned

Yes, this patch does still look viable. I don't know if changing this behavior warrants a deprecation cycle. The argument can be made that this would be a bug fix, not a behavior change.

We can bring it up at the next #ps (I probably won't attend, somebody else can mention it for me).

  Changed 3 years ago by whiteknight

  • priority changed from normal to trivial

I applied the most recent patch with Parrot_lib_search_paths_as_string. That function isn't wired in yet, but we will find use for it.

I'm not going to apply any other patches from this ticket yet, because I am trying to deprecate the current load_bytecode op and replace it with a more full-featured alternative. The new version will certainly throw exceptions instead of silent failures, so if the user wants non-bad behavior they can upgrade.

  Changed 3 years ago by whiteknight

I proposed the load_bytecode op for deprecation in #2146. If that deprecation is accepted, I'll close this ticket and redirect all further information there.

Note: See TracTickets for help on using tickets.