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

Fail with typed errors if nqp::open dies in IO::Handle.open. #894

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cygx
Copy link
Contributor

@cygx cygx commented Oct 4, 2016

Also removes an expensive check necessary to avoid opening directories:
This is now handled at the VM level, which still needs to happen on the JVM (cf Raku/nqp#310).

It could be made even nicer if there was a way to communicate error types to the HLL, eg via error codes provided in addition to the error message. Currently, we're forced to (non-atomically) repeat checks that already have been performed at the VM level (or parse error messages).

Also removes an expensive check necessary to avoid opening directories:
This is now handled at the VM level, which still needs to happen on the JVM
(cf Raku/nqp#310).
@cygx cygx reopened this Oct 4, 2016
@niner
Copy link
Collaborator

niner commented Oct 4, 2016

It's quite possible to throw a typed exception from the VM level as I've recently learned. It's just a little more involved. Look for P6EX in the source.

OTOH the additional checks are done only in an error case anyway. And how much performance are you gonna need there?

@cygx
Copy link
Contributor Author

cygx commented Oct 4, 2016

It's quite possible to throw a typed exception from the VM level as I've recently learned.

Wouldn't that introduce a dependency on the specific HLL?

And how much performance are you gonna need there?

It's not about performance, but having to duplicate the checks independently on the p6-side. Such a repetition just introduces another chance to get it wrong, which in fact I already managed to do (cf 191f68e).

It's also a bit iffy in so far as in principle, the file in question could have been modified between the failure of the call to MVM_file_open_fh() and the checks in the CATCH block, so in case of concurrent modification, it's theoretically possible to throw the wrong exception...

@zoffixznet zoffixznet self-assigned this Apr 13, 2017
@zoffixznet zoffixznet removed their assignment Feb 6, 2018
@zoffixznet
Copy link
Contributor

zoffixznet commented Feb 20, 2018

Is this PR still good or does its approach need to be changed entirely? Lack of untyped exceptions from open turned up here, today https://stackoverflow.com/questions/48865412/how-should-i-handle-perl-6-argfiles-that-cant-be-read-by-lines

@cygx
Copy link
Contributor Author

cygx commented Feb 20, 2018

Conflicts aside, it should still be good(-ish). The #?if jvm can likely go due to Raku/nqp#310 getting resolved. The new exception type could use a better name (DoesAlreadyExist? something completely different?). It would still be preferable if the reason for the failure could be communicated upwards from the VM to avoid race conditions (eg nqp::open fails due to missing file, file gets created between the attempt to open and the checking of the error conditions)...

@zoffixznet
Copy link
Contributor

It would still be preferable if the reason for the failure could be communicated upwards from the VM to avoid race conditions

Oh, whoops. Yeah, I've just actually looked at the code and not too happy with such decisions what what exceptions to throw. There's definitely a way to throw typed VM exceptions... I'll look into that.

@JJ JJ changed the base branch from nom to master May 3, 2020 11:12
Copy link
Collaborator

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is introducing a new class, as well as proposing a new (kinda) feature. So there're a few things that need to be done

  1. Fix the conflicts
  2. Provide an use case for it
  3. Turn that use case into a test, and submit it to roast.

Then we can discuss if this is really needed, in the problem-solving repo if necessary. Thanks anyway for the PR.

@lizmat
Copy link
Contributor

lizmat commented Jan 12, 2021

I am unsure as to the status of this PR. Can someone enlighten?

@niner
Copy link
Collaborator

niner commented Jan 12, 2021

The #?if jvm can go. It would indeed be better if the VM threw the typed exception. The mechanism for this does not create a dependency on the HLL (it's already in use in several places after all without trouble). Doing those checks twice does indeed create a race condition where the cause of the error may change between the checks. Nevertheless, if no one volunteers to implement throwing typed exceptions at the VM level, then we'd still be better off with this merged. Race condition and all.

@vrurg
Copy link
Member

vrurg commented Jan 12, 2021

The #?if jvm can go. It would indeed be better if the VM threw the typed exception. The mechanism for this does not create a dependency on the HLL (it's already in use in several places after all without trouble).

What kind of mechanism do you mean here?

@niner
Copy link
Collaborator

niner commented Jan 12, 2021 via email

@vrurg
Copy link
Member

vrurg commented Jan 12, 2021

@niner I have implemented a replacement for P6EX:

method throw_or_die($exception, $die_message, *@pos, *%named) {

When used it looks much tidier and readable than P6EX:

Perl6::Metamodel::Configuration.throw_or_die(
. And it doesn't require extra boilerplate to register an exception which is looked up by its name by traversing the stash of package X.

But use it from inside a VM would still be complicated by requiring named parameters for exception constructor.

@lizmat
Copy link
Contributor

lizmat commented Jan 5, 2022

Is this PR still salvageable ?

@coke coke changed the base branch from master to main April 19, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants