Exception handling bug; NameLookupFailure>defaultAction

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Exception handling bug; NameLookupFailure>defaultAction

timrowledge
I was taking a swing at fixing the FileBrowser bug caused by the non-existence of st.cs.uiuc.edu wherein we get a very annoying loop of notifiers and a hard time trying to get out of them. Looking around the related code suggests we really want to clean up a lot of methods to do with serverdirectory, ftpurl etc.

The error handling bug is quite simple to trigger for you pleasure and amusement (I'm past being amused by it and have gone around enough times that I think I can no longer see the wood for the trees)

 NetNameResolver addressForName: 'st.cs.uiuc.edu' timeout: 10.

should do it for you.
The problem is that we get to NameLookupFailure>defaultAction (because there is no other handler), which uses a UI to ask if the user wants to try again (sigh; raising a UI from a system error...) and if the option to retry is picked then it sends #restart to the handlerContext - which is not set. So far as I can see handlerContext only gets set by sending #privHandlerContext: which is only done just *after* the code that we just ran to get into trouble. With no set handlerContext we're also in trouble if we want to #pass, #return: or even just query #isNested.

If we use a 'deliberate' handler it works as one might hope
 [NetNameResolver addressForName: 'st.cs.uiuc.edu' timeout: 10] on: NameLookupFailure do:[:ex| ex pass].
So, maybe an edge case that got over-optimised? Like I said, my brain is now useless for this and it needs another set of eyes.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: FSRA: Forms Skip and Run-Away



Reply | Threaded
Open this post in threaded view
|

Re: Exception handling bug; NameLookupFailure>defaultAction

Frank Shearar-3

described the fun I had with that method nearly exactly 8 years ago!

frank

On Mon, Dec 31, 2018, 16:48 tim Rowledge <[hidden email] wrote:
I was taking a swing at fixing the FileBrowser bug caused by the non-existence of st.cs.uiuc.edu wherein we get a very annoying loop of notifiers and a hard time trying to get out of them. Looking around the related code suggests we really want to clean up a lot of methods to do with serverdirectory, ftpurl etc.

The error handling bug is quite simple to trigger for you pleasure and amusement (I'm past being amused by it and have gone around enough times that I think I can no longer see the wood for the trees)

 NetNameResolver addressForName: 'st.cs.uiuc.edu' timeout: 10.

should do it for you.
The problem is that we get to NameLookupFailure>defaultAction (because there is no other handler), which uses a UI to ask if the user wants to try again (sigh; raising a UI from a system error...) and if the option to retry is picked then it sends #restart to the handlerContext - which is not set. So far as I can see handlerContext only gets set by sending #privHandlerContext: which is only done just *after* the code that we just ran to get into trouble. With no set handlerContext we're also in trouble if we want to #pass, #return: or even just query #isNested.

If we use a 'deliberate' handler it works as one might hope
 [NetNameResolver addressForName: 'st.cs.uiuc.edu' timeout: 10] on: NameLookupFailure do:[:ex| ex pass].
So, maybe an edge case that got over-optimised? Like I said, my brain is now useless for this and it needs another set of eyes.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: FSRA: Forms Skip and Run-Away





Reply | Threaded
Open this post in threaded view
|

Re: Exception handling bug; NameLookupFailure>defaultAction

timrowledge


> On 2018-12-31, at 9:28 PM, Frank Shearar <[hidden email]> wrote:
>
> https://tech.labs.oliverwyman.com/blog/2011/01/04/try-again-with-exceptions/
>
> described the fun I had with that method nearly exactly 8 years ago!

An alarmingly familiar feeling. It's almost as if bugs never actually get solved. Weirdest one I can recall is when working for Interval Research I had to fix some Squeak VM thing to do with file handling; it seemed ever so familiar and I dug out notebooks and discovered that exactly 10 years (to the week) before I had been fixing the same problem in the same primitive but in a totally different VM.

Just for fun and masochism, I tried another couple of tests this morning. If you add the UIUC not-site with
        | aa |
        self flag: #ViolateNonReferenceToOtherClasses.
        aa := ServerDirectory new.
        aa server: 'st.cs.uiuc.edu'.    "host"
        aa user: 'anonymous'.
        aa password: '[hidden email]'.
        aa directory: '/Smalltalk/Squeak/Goodies'.
        aa url: ''.    "<- this is optional.  Only used when *writing* update files."
        ServerDirectory addServer: aa named: 'UIUCArchive'.  "<- known by this name in Squeak"
and then open a filelist & select the UIUCArchive directory, 'give up' opens a notifier, which looks more like it.

Except... try debugging by clicking on a item in the stack listing or the 'debug' button - it's back to the never ending loop. That was a bit of a surprise. Even more of a surprise was that somehow after going around a few times it crashed the VM with no log and even more surprisingly restarting the image to attempt replication went back to seeming to work 'properly' ie opening a debugger.

Clearly what ought to happen is a dNU in Exception>retry because of the unset handlerContext. I *think* a possible solution would be to set the handlerContext to 'thisContext' in UndefinedObject>handleSignal: and it *seems* to work for me so far. This is definitely *not* something I'm going to stick in trunk without serious input.
UndefinedObject>handleSignal: exception
        "When no more handler (on:do:) context left in sender chain this gets called.  Return from signal with default action."
        exception privHandlerContext: thisContext.
        ^ exception resumeUnchecked: exception defaultAction

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
How many of you believe in telekinesis? Raise my hands....



Reply | Threaded
Open this post in threaded view
|

Re: Exception handling bug; NameLookupFailure>defaultAction

timrowledge
In reply to this post by timrowledge
Exception handling is fun!

After some discussions at todays' SOB meeting, Eliot pointed out that we should be worrying about what the J20 standard says about exceptions. Interestingly, it explicitly calls out the sort of thing that NameLookupFailure>defaultAction does as wrong, with capital W and extra '!!'. So, even though I found a slightly tacky work-around that appears to make it function most of the time, what we actually need to do is fix the real problem, ie bad code in some implementations of #defaultAction. And, maybe, comment a few places to help future-us get it right later.

Looking at implementations of #defaultAction in the image I see the following in need of fixing -

EnvironmentRequest>>#defaultAction
FTPConnectionException>>#defaultAction
InMidstOfFileinNotification>>#defaultAction
MCProxyMaterialization>>#defaultAction
NameLookupFailure>>#defaultAction
OutOfScopeNotification>>#defaultAction
ProgressInitiationException>>#defaultAction
ProgressTargetRequestNotification>>#defaultAction
ProjectEntryNotification>>#defaultAction
ProjectPasswordNotification>>#defaultAction
ProjectViewOpenNotification>>#defaultAction
RequestAlternateSyntaxSetting>>#defaultAction (note that this was related to an ancient syntax experiment and the involved preference is no longer implemented and should be finally excised - see #printAlternateSyntax)
WebAuthRequired>>#defaultAction

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: START: Cancel preceding jobs in queue



Reply | Threaded
Open this post in threaded view
|

Re: Exception handling bug; NameLookupFailure>defaultAction

marcel.taeumel
Hi, there.

I suppose that in all cases the fix is like this:

OLD:
"some stuff"
MyException signal.
"more stuff"

NEW:
["some stuff" MyException signal]
  on: MyException
  do: [:ex | ex retry].
"more stuff"

Plus, removing that #retry call from the execptions #defaultAction implementation. Similar for #pass, #resume, #resume:, #retryUsing:, #return, #return:.

Best,
Marcel

Am 03.01.2019 06:05:36 schrieb tim Rowledge <[hidden email]>:

Exception handling is fun!

After some discussions at todays' SOB meeting, Eliot pointed out that we should be worrying about what the J20 standard says about exceptions. Interestingly, it explicitly calls out the sort of thing that NameLookupFailure>defaultAction does as wrong, with capital W and extra '!!'. So, even though I found a slightly tacky work-around that appears to make it function most of the time, what we actually need to do is fix the real problem, ie bad code in some implementations of #defaultAction. And, maybe, comment a few places to help future-us get it right later.

Looking at implementations of #defaultAction in the image I see the following in need of fixing -

EnvironmentRequest>>#defaultAction
FTPConnectionException>>#defaultAction
InMidstOfFileinNotification>>#defaultAction
MCProxyMaterialization>>#defaultAction
NameLookupFailure>>#defaultAction
OutOfScopeNotification>>#defaultAction
ProgressInitiationException>>#defaultAction
ProgressTargetRequestNotification>>#defaultAction
ProjectEntryNotification>>#defaultAction
ProjectPasswordNotification>>#defaultAction
ProjectViewOpenNotification>>#defaultAction
RequestAlternateSyntaxSetting>>#defaultAction (note that this was related to an ancient syntax experiment and the involved preference is no longer implemented and should be finally excised - see #printAlternateSyntax)
WebAuthRequired>>#defaultAction

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: START: Cancel preceding jobs in queue





Reply | Threaded
Open this post in threaded view
|

Re: Exception handling bug; NameLookupFailure>defaultAction

Ben Coman
In reply to this post by timrowledge


On Thu, 3 Jan 2019 at 13:05, tim Rowledge <[hidden email]> wrote:
Exception handling is fun!

After some discussions at todays' SOB meeting, Eliot pointed out that we should be worrying about what the J20 standard says about exceptions. Interestingly, it explicitly calls out the sort of thing that NameLookupFailure>defaultAction does as wrong, with capital W and extra '!!'.

but I don't see "capital W and extra '!!'"

cheers -ben

 
So, even though I found a slightly tacky work-around that appears to make it function most of the time, what we actually need to do is fix the real problem, ie bad code in some implementations of #defaultAction. And, maybe, comment a few places to help future-us get it right later.

Looking at implementations of #defaultAction in the image I see the following in need of fixing -

EnvironmentRequest>>#defaultAction
FTPConnectionException>>#defaultAction
InMidstOfFileinNotification>>#defaultAction
MCProxyMaterialization>>#defaultAction
NameLookupFailure>>#defaultAction
OutOfScopeNotification>>#defaultAction
ProgressInitiationException>>#defaultAction
ProgressTargetRequestNotification>>#defaultAction
ProjectEntryNotification>>#defaultAction
ProjectPasswordNotification>>#defaultAction
ProjectViewOpenNotification>>#defaultAction
RequestAlternateSyntaxSetting>>#defaultAction (note that this was related to an ancient syntax experiment and the involved preference is no longer implemented and should be finally excised - see #printAlternateSyntax)
WebAuthRequired>>#defaultAction

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: START: Cancel preceding jobs in queue





Reply | Threaded
Open this post in threaded view
|

Re: Exception handling bug; NameLookupFailure>defaultAction

Jakob Reschke-2
In reply to this post by timrowledge
The following are resumable, so the resume: in defaultAction just needs to be replaced by a return from the method:
EnvironmentRequest
FTPConnectionException  
InMidstOfFileinNotification
MCProxyMaterialization
OutOfScopeNotification
ProgressInitiationException
ProgressTargetRequestNotification
ProjectEntryNotification
ProjectPasswordNotification
ProjectViewOpenNotification
RequestAlternateSyntaxSetting
WebAuthRequired

Some of their defaultActions could rather answer their defaultResumeValue, so it is not duplicated.

The only one remaining that needs more thinking about is NameLookupFailure.

The hint that one must/should not control the signal handling from defaultAction could be added to Exception>defaultAction and Notification>defaultAction.

Am Do., 3. Jan. 2019 um 06:06 Uhr schrieb tim Rowledge <[hidden email]>:
Exception handling is fun!

After some discussions at todays' SOB meeting, Eliot pointed out that we should be worrying about what the J20 standard says about exceptions. Interestingly, it explicitly calls out the sort of thing that NameLookupFailure>defaultAction does as wrong, with capital W and extra '!!'. So, even though I found a slightly tacky work-around that appears to make it function most of the time, what we actually need to do is fix the real problem, ie bad code in some implementations of #defaultAction. And, maybe, comment a few places to help future-us get it right later.

Looking at implementations of #defaultAction in the image I see the following in need of fixing -

EnvironmentRequest>>#defaultAction
FTPConnectionException>>#defaultAction
InMidstOfFileinNotification>>#defaultAction
MCProxyMaterialization>>#defaultAction
NameLookupFailure>>#defaultAction
OutOfScopeNotification>>#defaultAction
ProgressInitiationException>>#defaultAction
ProgressTargetRequestNotification>>#defaultAction
ProjectEntryNotification>>#defaultAction
ProjectPasswordNotification>>#defaultAction
ProjectViewOpenNotification>>#defaultAction
RequestAlternateSyntaxSetting>>#defaultAction (note that this was related to an ancient syntax experiment and the involved preference is no longer implemented and should be finally excised - see #printAlternateSyntax)
WebAuthRequired>>#defaultAction

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: START: Cancel preceding jobs in queue