Fix Issue 11338 - std.uri URIerror should be an Exception #1659

DannyArends · 2013-10-24T09:54:17Z

URIerror renamed to URIException
URIException class inherits from Exception
Introduces human readable errors for URI parsing
What to do when alloca returns NULL

throw new OutOfMemoryError("Alloca failure");

- URIerror renamed to URIexception - URIexception class inherits from Exception

ghost · 2013-10-24T11:25:16Z

std/uri.d

@@ -116,7 +116,7 @@ private string URI_Encode(dstring string, uint unescapedSet)
                {
                    R2 = cast(char *)alloca(Rsize * char.sizeof);
                    if (!R2)


Uhm.. As far as I can tell alloca doesn't return 0 like a regular malloc call, it's usually documented as:

alloca() returns a pointer to the beginning of the allocated space. If the allocation causes stack overflow, program behaviour is undefined.

So I'm not sure why there was a check here. But in any case, this is an error condition, not a regular exception.

To be perfectly pedantic, if alloca returns null then the appropriate exception should be something like AllocaError (rather plain Error to be realistic), as it has no defined cross-platform meaning. However, it's likely that the platforms that do return null from alloca mean it to be OOM, so calling core.exception.onOutOfMemoryError might be the right move, even though in certain programs it might result in misguided attempts at freeing heap memory then retrying, which may or may not run out of stack space again.

Agreed. if alloca returns 0, then an error needs to be thrown. I think it would be wrong to call onOutOfMemoryError, since we aren't strictly dealing with memory, but rather, throw a straight up throw new OutOfMemoryError("Alloca Failure"). But I'm not certain about this.

Rest of the pull looks good (only gave it a quick glance though).

a quick grep -r "alloca(" in phobos shows that in general the return value (0/NULL) is ignored:
stream.d: No check for NULL return value
process.d: No check for NULL return value
typecons.d: No check for NULL return value

MSDN says they use SEH exceptions when alloca fails, whereas for DMC/Posix I'm not really sure what alloca does if it fails.

DannyArends · 2013-10-24T11:40:41Z

True, didn't spot the allocation.

Should it be separated then into an Error on allocation failure
and an Exception then for the checking of the input URI ?

ghost · 2013-10-24T11:52:50Z

Should it be separated then into an Error on allocation failure
and an Exception then for the checking of the input URI ?

I think an OutOfMemoryError should be thrown (from core.exception). I'd like to wait for others' input on this, @monarchdodra @alexrp etc.

As for the exception, it should also give some information as to what went wrong , "URI error" gives no information on what went wrong.

JakobOvrum · 2013-10-24T11:55:54Z

std/uri.d

@@ -36,7 +36,7 @@ private import std.c.stdlib;
 private import std.utf;
 import std.exception;

-class URIerror : Error
+class URIexception : Exception


I believe the correct name would be URIException, i.e. with a capital "E". (the old name was wrong too)

DannyArends · 2013-10-24T11:56:08Z

OK

Thanks for your time,
I'm more then willing to revise the pull based on the outcome of your discussion.

ps. I made a forum post also: http://forum.dlang.org/thread/xpiewrackieqynsmxmlb@forum.dlang.org

ghost · 2013-10-24T12:02:53Z

Yep, saw it. Thanks.

- Now call the onOutOfMemoryError() function when alloca returns NULL - Fixed the URIexception name to URIException - Added a string constructor for the URIException class - Added more informative messages to some of the errors

- Now all thrown URIException have human readable text - throw new OutOfMemoryError("Alloca Failure"); // Make code look more consistant

DannyArends · 2013-10-26T08:30:09Z

Hey people,

Is there yet any consensus on how to handle the alloca errors ?

The behaviour on an alloca failure only changes slightly with this fix.. The function used to throw an
URIerror which was/is obviously wrong,

I changed it to throw OutOfMemoryError("Alloca Failure"), which is more descriptive then the previously thrown URIerror

I think this is more or less the best we can do now (seeing as there is no default way to handle alloca failure under posix/unix like systems.

monarchdodra · 2013-11-01T08:35:28Z

std/uri.d

        if (!isHexDigit(s[k + 1]) || !isHexDigit(s[k + 2]))
-            goto LthrowURIerror;
+            throw new URIException("Expected HexDigit After '%'");


"Expected two HexDigits After '%'" also, english words: "Expected two hexadecimal digits After '%'"

monarchdodra · 2013-11-01T08:36:15Z

LGTM appart from those two last nitpicks. Will pull after you fix and it greens.

yazd · 2013-11-01T08:40:38Z

This is also a nitpick, but wouldn't sentence casing be better for the messages rather than the capitalization of each word?
e.g. UTF-32 Code Point Size Too Large => UTF-32 code point size too large

… for missing hexadecimals

DannyArends · 2013-11-01T10:44:40Z

There is a failure in the autotester (FreeBSD_64):

Testing std.parallelism: FAIL
core.thread.ThreadException@src/core/thread.d(935): Unable to set thread priority

Is this related to my commits ? Seems hardly possible

monarchdodra · 2013-11-01T10:52:06Z

Is this related to my commits ? Seems hardly possible

Nope, it's a failure that happens from time to time. It's not your fault. Don't worry about, we know about it.

ghost · 2013-11-01T15:50:34Z

Yes, LGTM too. We'll have to wait for the next test-run cycle so it greens.

Sorry for the delay @DannyArends, we'll merge your first pull soon enough. :)

Fix Issue 11338 - std.uri URIerror should be an Exception

monarchdodra · 2013-11-01T16:20:24Z

TYVM for putting up with us :)

Fix of Issue 11338

1fd19c2

- URIerror renamed to URIexception - URIexception class inherits from Exception

ghost reviewed Oct 24, 2013
View reviewed changes

JakobOvrum reviewed Oct 24, 2013
View reviewed changes

DannyArends added 2 commits October 24, 2013 14:30

Fix Issue 11338

fa0627b

- Now call the onOutOfMemoryError() function when alloca returns NULL - Fixed the URIexception name to URIException - Added a string constructor for the URIException class - Added more informative messages to some of the errors

Fixing Issue 11338 - Additional

74d59dd

- Now all thrown URIException have human readable text - throw new OutOfMemoryError("Alloca Failure"); // Make code look more consistant

monarchdodra reviewed Nov 1, 2013
View reviewed changes

Fixing exception casing to line style, added better exception message…

d7fa8ad

… for missing hexadecimals

monarchdodra added a commit that referenced this pull request Nov 1, 2013

Merge pull request #1659 from DannyArends/issue_11338

42eec50

Fix Issue 11338 - std.uri URIerror should be an Exception

monarchdodra merged commit 42eec50 into dlang:master Nov 1, 2013

Uh oh!

Fix Issue 11338 - std.uri URIerror should be an Exception #1659

Fix Issue 11338 - std.uri URIerror should be an Exception #1659

Uh oh!

Conversation

DannyArends commented Oct 24, 2013

Uh oh!

ghost Oct 24, 2013

Choose a reason for hiding this comment

Uh oh!

JakobOvrum Oct 24, 2013

Choose a reason for hiding this comment

Uh oh!

monarchdodra Oct 24, 2013

Choose a reason for hiding this comment

Uh oh!

DannyArends Oct 24, 2013

Choose a reason for hiding this comment

Uh oh!

ghost Oct 24, 2013

Choose a reason for hiding this comment

Uh oh!

DannyArends commented Oct 24, 2013

Uh oh!

ghost commented Oct 24, 2013

Uh oh!

JakobOvrum Oct 24, 2013

Choose a reason for hiding this comment

Uh oh!

DannyArends commented Oct 24, 2013

Uh oh!

ghost commented Oct 24, 2013

Uh oh!

DannyArends commented Oct 26, 2013

Uh oh!

monarchdodra Nov 1, 2013

Choose a reason for hiding this comment

Uh oh!

monarchdodra commented Nov 1, 2013

Uh oh!

yazd commented Nov 1, 2013

Uh oh!

DannyArends commented Nov 1, 2013

Uh oh!

monarchdodra commented Nov 1, 2013

Uh oh!

ghost commented Nov 1, 2013

Uh oh!

monarchdodra commented Nov 1, 2013

Uh oh!

Uh oh!