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
require should not clobber $! and $^E on success #13219
Comments
From cm.perl@abtela.comCreated by cm.perl@abtela.comOn success, require resets In the process of foraging through @INC, require may try to open several But resetting sub croak { The problem here is that on first invocation (assuming Carp has not been Taisha:/cygdrive/d/perls/blead/perl-git/win32 $ cat sub croak { for (1 .. 3) { Recent work on Carp (commit cbd58ba) In line with this effort, and considering the problem outlined above, ALTERNATIVE If there is a compelling reason to keep having require corrupt^Hreset sub croak { here is an indicative list of core files to patch : WORKAROUND The problem above can be avoided by loading Carp as early as possible in Perl Info
|
From cm.perl@abtela.comFist a big thanks to whoever took the pain to properly resend this Le 02/09/2013 12:55, Zefram a écrit :
Damn. I have spent ages trying to try and craft this ticket so to avoid If the pseudocode in the pod is to be trusted, require signals errors by I am specifically *not* advocating using "local ( sub require { which restores Le 02/09/2013 13:40, Zefram a écrit :
for those on p5p who wonder what the hell we are talking about, and To answer your question, my main focus with #116118 has been on trying What I am really after is the following strategy for programmatic error eval { some_sub_that_might_die_or_croak; 1 } or do { [the underlying issue being that it is often impossible to make sense of Your "local ( One remaining problem is as follows:
Once this is solved I believe my concerns with error reporting will have As a general rule, I think ops and subs and constructs should not In the specific case of require, I believe that fixing it as suggested Christian |
From cm.perl@abtela.comLe 02/09/2013 11:28, Christian Millour (via RT) a écrit :
Let me restate this, and add some motivation. To recall, the problem Let us start with the baseline sample using die :
So far so good. We are taught about using Carp::croak instead of die when authoring
what the croaking code cannot do however is 'autouse Carp', even though
The problem arises through no fault of Carp or autouse, but because
but that pretty much defeats the purpose of autouse, and makes for a package Whatever; suffers from the same problem. Possibly more serious, given e.g. how the latest edition of "Modern
again through no fault of autodie, but for the same reason as before
but is fragile and IMHO raises obscurity to a new level of darkness :-/ Actually, if the croaking code uses autodie, then the client code may
unfortunately the doc for errno in autodie::exception states that 'This On some systems ----------- SO ? -------------- I can't think of a compelling reason for require to reset $! (and Could this tentative patch please be updated as needed and Thanks for your time and consideration. Christian |
From @craigberryOn Wed, Jul 23, 2014 at 4:00 AM, Christian Millour <cm.perl@abtela.com> wrote:
The problem is my patch doesn't actually do what you want. It does But after it's opened, the file is sent to S_doeval to be compiled, On the surface, it sounds reasonable that a successful require should For future reference, here's what you get with my patch: $ type foo.t
So you can see that errno is still cleared in require even though I |
From @craigberry0001-Restore-errno-after-INC-search-in-pp_require.patchFrom 39094c4e6cf941efd0be9bc0d77eda99230dfd05 Mon Sep 17 00:00:00 2001
From: "Craig A. Berry" <craigberry@mac.com>
Date: Sat, 26 Jul 2014 12:44:42 -0500
Subject: [PATCH] Restore errno after @INC search in pp_require.
This addresses [perl #119555]. It's quite natural when looking
through @INC for a module that we don't always find it in the first
place we look and errno will have been set by a failed filesystem
lookup. Historically we've always cleared errno when the lookup
is eventually successful so that we don't have a misleading value
hanging around.
This patch changes that behavior so we restore errno to what it was
before starting the @INC search rather than clearing it.
---
pp_ctl.c | 11 ++++++-----
t/op/require_errors.t | 10 +++++++++-
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/pp_ctl.c b/pp_ctl.c
index 7d098b7..887e05d 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3676,8 +3676,9 @@ PP(pp_require)
SV *hook_sv = NULL;
SV *encoding;
OP *op;
- int saved_errno;
+ int doopen_errno;
bool path_searchable;
+ dSAVE_ERRNO;
sv = POPs;
if ( (SvNIOKp(sv) || SvVOK(sv)) && PL_op->op_type != OP_DOFILE) {
@@ -4022,13 +4023,13 @@ PP(pp_require)
}
}
}
- saved_errno = errno; /* sv_2mortal can realloc things */
+ doopen_errno = errno; /* sv_2mortal can realloc things */
sv_2mortal(namesv);
if (!tryrsfp) {
if (PL_op->op_type == OP_REQUIRE) {
- if(saved_errno == EMFILE || saved_errno == EACCES) {
+ if(doopen_errno == EMFILE || doopen_errno == EACCES) {
/* diag_listed_as: Can't locate %s */
- DIE(aTHX_ "Can't locate %s: %s", name, Strerror(saved_errno));
+ DIE(aTHX_ "Can't locate %s: %s", name, Strerror(doopen_errno));
} else {
if (namesv) { /* did we lookup @INC? */
AV * const ar = GvAVn(PL_incgv);
@@ -4072,7 +4073,7 @@ PP(pp_require)
RETPUSHUNDEF;
}
else
- SETERRNO(0, SS_NORMAL);
+ RESTORE_ERRNO;
/* Assume success here to prevent recursive requirement. */
/* name is never assigned to again, so len is still strlen(name) */
diff --git a/t/op/require_errors.t b/t/op/require_errors.t
index a152d2d..09a7d67 100644
--- a/t/op/require_errors.t
+++ b/t/op/require_errors.t
@@ -7,7 +7,7 @@ BEGIN {
require './test.pl';
}
-plan(tests => 17);
+plan(tests => 19);
my $nonfile = tempfile();
@@ -133,3 +133,11 @@ like $@, qr/^Can't locate strict\.pm\\0invalid: /, 'do nul check';
eval "require strict\0::invalid;";
like $@, qr/^syntax error at \(eval \d+\) line 1/, 'parse error with \0 in barewords module names';
+{
+ local ($!, $^E) = my ($e, $se) = (1, 1);
+ # Should look in 'Perl', then 'Rules' (unsuccessfully), then '../lib'
+ require 'version.pm';
+ is (0+$! , $e , q{require does not obviously clobber $!});
+ is (0+$^E, $se, q{require does not obviously clobber $^E})
+ or diag ("BTW, " . (0+$^E) . " is '$^E'");
+}
--
1.8.4.2
|
The RT System itself - Status changed from 'new' to 'open' |
From cm.perl@abtela.comLe 28/07/2014 05:01, Craig A. Berry a écrit :
Thank you for your time and efforts on this. pp_require is indeed quite involved and I fully understand anyone being Still, according to the documentation, require dies on error, and sub new_require { While not strictly equivalent, and as an experiment, I wrapped the PP(pp_require1) However crude and ill-mannered, this hack seemed to work. A variation on This seems to work (one.pm is successfully required, without clobbering blead(w32) $ echo 0 > zero.pm; echo 1 > one.pm This is better than the stock perl (here strawberry perl portable 5.20) Perl_5.20.0 $ echo 0 > zero.pm; echo 1 > one.pm In case of failure the values of Comments/critics/smokes warmly welcome :-) Best regards, Christian |
From cm.perl@abtela.com0001-Restore-errno-on-succesfull-require-199555.patchFrom e6eea131fb645accf4e2d8cea693ad1c7709fbe7 Mon Sep 17 00:00:00 2001
From: Christian Millour <cm.perl@abtela.com>
Date: Mon, 28 Jul 2014 23:34:22 +0200
Subject: [PATCH] Restore errno on succesfull require (#199555)
Historically, require used to clear errno ($!) on success. While errno
(and on some platforms $^E) might be set several times during a
require (such as when searching through @INC and failing on the first
directories), such temporary errors should not be visible to the user
when require finally succeeds. Clearing errno is wrong however, as
this produces pernicious action-at-a-distance effects (see #199555).
Not doing anything, as was the case with $^E, was no better as it left
$^E in a semi-random state. This patch addresses these issues by
saving $! and $^E on entry to require, and restoring their values on
successfull exit.
---
pp_ctl.c | 19 +++++++++++--------
1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/pp_ctl.c b/pp_ctl.c
index 7d098b7..9975be2 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3676,8 +3676,9 @@ PP(pp_require)
SV *hook_sv = NULL;
SV *encoding;
OP *op;
- int saved_errno;
+ int doopen_errno;
bool path_searchable;
+ dSAVE_ERRNO;
sv = POPs;
if ( (SvNIOKp(sv) || SvVOK(sv)) && PL_op->op_type != OP_DOFILE) {
@@ -3735,6 +3736,7 @@ PP(pp_require)
}
}
+ RESTORE_ERRNO;
RETPUSHYES;
}
name = SvPV_const(sv, len);
@@ -3777,9 +3779,10 @@ PP(pp_require)
SV * const * const svp = hv_fetch(GvHVn(PL_incgv),
unixname, unixlen, 0);
if ( svp ) {
- if (*svp != &PL_sv_undef)
+ if (*svp != &PL_sv_undef) {
+ RESTORE_ERRNO;
RETPUSHYES;
- else
+ } else
DIE(aTHX_ "Attempt to reload %s aborted.\n"
"Compilation failed in require", unixname);
}
@@ -4022,13 +4025,13 @@ PP(pp_require)
}
}
}
- saved_errno = errno; /* sv_2mortal can realloc things */
+ doopen_errno = errno; /* sv_2mortal can realloc things */
sv_2mortal(namesv);
if (!tryrsfp) {
if (PL_op->op_type == OP_REQUIRE) {
- if(saved_errno == EMFILE || saved_errno == EACCES) {
+ if(doopen_errno == EMFILE || doopen_errno == EACCES) {
/* diag_listed_as: Can't locate %s */
- DIE(aTHX_ "Can't locate %s: %s", name, Strerror(saved_errno));
+ DIE(aTHX_ "Can't locate %s: %s", name, Strerror(doopen_errno));
} else {
if (namesv) { /* did we lookup @INC? */
AV * const ar = GvAVn(PL_incgv);
@@ -4069,10 +4072,9 @@ PP(pp_require)
}
CLEAR_ERRSV();
+ RESTORE_ERRNO;
RETPUSHUNDEF;
}
- else
- SETERRNO(0, SS_NORMAL);
/* Assume success here to prevent recursive requirement. */
/* name is never assigned to again, so len is still strlen(name) */
@@ -4131,6 +4133,7 @@ PP(pp_require)
LOADED_FILE_PROBE(unixname);
+ RESTORE_ERRNO;
return op;
}
--
1.7.4
|
From cm.perl@abtela.comminor edit on the patch file to reference the proper RT ticket (119... Christian Le 29/07/2014 00:46, Christian Millour a écrit :
|
From cm.perl@abtela.com0001-Restore-errno-on-succesfull-require-119555.patchFrom ef0be7397327c777298a079d398b86062869b1b6 Mon Sep 17 00:00:00 2001
From: Christian Millour <cm.perl@abtela.com>
Date: Mon, 28 Jul 2014 23:34:22 +0200
Subject: [PATCH] Restore errno on succesfull require (#119555)
Historically, require used to clear errno ($!) on success. While errno
(and on some platforms $^E) might be set several times during a
require (such as when searching through @INC and failing on the first
directories), such temporary errors should not be visible to the user
when require finally succeeds. Clearing errno is wrong however, as
this produces pernicious action-at-a-distance effects (see #119555).
Not doing anything, as was the case with $^E, was no better as it left
$^E in a semi-random state. This patch addresses these issues by
saving $! and $^E on entry to require, and restoring their values on
successfull exit.
---
pp_ctl.c | 19 +++++++++++--------
1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/pp_ctl.c b/pp_ctl.c
index 7d098b7..9975be2 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3676,8 +3676,9 @@ PP(pp_require)
SV *hook_sv = NULL;
SV *encoding;
OP *op;
- int saved_errno;
+ int doopen_errno;
bool path_searchable;
+ dSAVE_ERRNO;
sv = POPs;
if ( (SvNIOKp(sv) || SvVOK(sv)) && PL_op->op_type != OP_DOFILE) {
@@ -3735,6 +3736,7 @@ PP(pp_require)
}
}
+ RESTORE_ERRNO;
RETPUSHYES;
}
name = SvPV_const(sv, len);
@@ -3777,9 +3779,10 @@ PP(pp_require)
SV * const * const svp = hv_fetch(GvHVn(PL_incgv),
unixname, unixlen, 0);
if ( svp ) {
- if (*svp != &PL_sv_undef)
+ if (*svp != &PL_sv_undef) {
+ RESTORE_ERRNO;
RETPUSHYES;
- else
+ } else
DIE(aTHX_ "Attempt to reload %s aborted.\n"
"Compilation failed in require", unixname);
}
@@ -4022,13 +4025,13 @@ PP(pp_require)
}
}
}
- saved_errno = errno; /* sv_2mortal can realloc things */
+ doopen_errno = errno; /* sv_2mortal can realloc things */
sv_2mortal(namesv);
if (!tryrsfp) {
if (PL_op->op_type == OP_REQUIRE) {
- if(saved_errno == EMFILE || saved_errno == EACCES) {
+ if(doopen_errno == EMFILE || doopen_errno == EACCES) {
/* diag_listed_as: Can't locate %s */
- DIE(aTHX_ "Can't locate %s: %s", name, Strerror(saved_errno));
+ DIE(aTHX_ "Can't locate %s: %s", name, Strerror(doopen_errno));
} else {
if (namesv) { /* did we lookup @INC? */
AV * const ar = GvAVn(PL_incgv);
@@ -4069,10 +4072,9 @@ PP(pp_require)
}
CLEAR_ERRSV();
+ RESTORE_ERRNO;
RETPUSHUNDEF;
}
- else
- SETERRNO(0, SS_NORMAL);
/* Assume success here to prevent recursive requirement. */
/* name is never assigned to again, so len is still strlen(name) */
@@ -4131,6 +4133,7 @@ PP(pp_require)
LOADED_FILE_PROBE(unixname);
+ RESTORE_ERRNO;
return op;
}
--
1.7.4
|
This patch no longer applies. Does anyone else have an idea or do we call this a "feature" of Windows? |
Migrated from rt.perl.org#119555 (status was 'open')
Searchable as RT119555$
The text was updated successfully, but these errors were encountered: