Smalltalk › Instantiations

SstSmtpAddress class: bad exception handling

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

8 messages Options

jtuchel

SstSmtpAddress class: bad exception handling

Hi there,

Before I forget it: VAST 8.5 on Win XP and Linux.

There is a really ugly problem in SstSmtpAddress class>>setHost:port:locally:for: that stems from the fact that SST is using return values rather than exceptions (like CFS, for example).

First, let's look at the method:
setHost: hostName port: port locally: locally for: address

    | host sockAddr |
    host := hostName.
    locally ifTrue: [
        sockAddr := SciSocketAddress generateInternetAddress: host port: port protocolType: 'tcp'.
        sockAddr isSciError ifTrue: [^SstError for: SstInvalidAddressError with: sockAddr].
        address socketAddress: sockAddr.
            "Must have a valid hostname for Message-Id. We could do this when building the
            Message-Id, but that would require a DNS call per message whereas this is a one-time
            deal."
        host := SstTcpLightTransport officialHostNameFor: sockAddr address].
    address
        host: host;
        port: port.
    ^address

The red line above doesn't check the return value of officialHostNameFor:. In case of an ENOADDRESS (e.g. due to firewall/proxy problems), and so makes it hard to catch problems from the outside. What you end up with is an SciError does not understand: trimBlanks in SstSmtpAddress>>host: in the blue statement above.

This is very hard to handle as a user of some sender of a sender of a sender of this method...

What should happen? If the Address cannot be resolved, I'd like to get an exception that I can use to react to this. A dNU is a very bad candidate for acting in any sensible way in this situation...

So what am I asking for?
Short term: ask for the result of officialHostNameFor: and react accordingly (don't continue).
Mid term: get rid of isSciError and friends and replace them with Exceptions.

Just my 2 cents.

Joachim

Joachim

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/fkMsXp36R58J.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.

John O'Keefe-3

Re: SstSmtpAddress class: bad exception handling

Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/mkZCsanWArAJ.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.

jtuchel

Re: SstSmtpAddress class: bad exception handling

Thanks, John.

I tried adding something like this:

host := SstTcpLightTransport officialHostNameFor: sockAddr address.

host isSciError ifTrue: [^SstError for: SstInvalidAddressError with: sockAddr address]].

It looks better now, but still I am not sure I like the result. I now have a chance to do an isSstError Check, but it is way more complicated than catching an exception. So what I'm doing is checking the result and signaling it in an exception in my caller code ;-) Not effective, and surely not elegant, but this way the code fits better into the overall picture...

I guess there's no way of getting you to comment on my mid-term wish (replacing error checks with exceptions). It would both make writing new code much easier and also break lots of existing code (maybe I've just answered my question myself... ;-) ). But maybe I am even unaware of any advantages of isXxxError checks throughout some of VAST's frameworks...

Joachim

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:

Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:

Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/6Nf5Nk-8nJIJ.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.

John O'Keefe-3

Re: SstSmtpAddress class: bad exception handling

Joachim -

What you tried is quite similar to what we will implement in V8.5.2.

The thought process behind using isXxxError instead of exceptions is lost in the mists of time (way before my time). Once the pattern was set in the low level frameworks (like CFS), it was used in most later frameworks.

John

On Friday, May 25, 2012 5:09:32 PM UTC-4, [hidden email] wrote:

Thanks, John.

I tried adding something like this:

host := SstTcpLightTransport officialHostNameFor: sockAddr address.
host isSciError ifTrue: [^SstError for: SstInvalidAddressError with: sockAddr address]].

It looks better now, but still I am not sure I like the result. I now have a chance to do an isSstError Check, but it is way more complicated than catching an exception. So what I'm doing is checking the result and signaling it in an exception in my caller code ;-) Not effective, and surely not elegant, but this way the code fits better into the overall picture...

I guess there's no way of getting you to comment on my mid-term wish (replacing error checks with exceptions). It would both make writing new code much easier and also break lots of existing code (maybe I've just answered my question myself... ;-) ). But maybe I am even unaware of any advantages of isXxxError checks throughout some of VAST's frameworks...

Joachim

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/L1AGKAYp3FYJ.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.

jtuchel

Re: SstSmtpAddress class: bad exception handling

Hi John,

I look forward to seeing 8.5.2.

Do you see any chance for such fundamental changes as getting rid of isXxxError checking any time soon? I know there's going to be a lot of code broken with such changes, but a mixture of exception handling and return value checking makes code harder to maintain and thus systems more brittle. Maybe a first step could be that "public APIs" of frameworks like CFS or Sst throw exceptions...

My guess would be that those checks stem from the way we were taught to program in C for Systems like GEM back in the eighties...

Joachim

Am Mittwoch, 30. Mai 2012 19:51:21 UTC+2 schrieb John O'Keefe:

Joachim -

What you tried is quite similar to what we will implement in V8.5.2.

The thought process behind using isXxxError instead of exceptions is lost in the mists of time (way before my time). Once the pattern was set in the low level frameworks (like CFS), it was used in most later frameworks.

John

On Friday, May 25, 2012 5:09:32 PM UTC-4, [hidden email] wrote:
Thanks, John.

I tried adding something like this:

host := SstTcpLightTransport officialHostNameFor: sockAddr address.
host isSciError ifTrue: [^SstError for: SstInvalidAddressError with: sockAddr address]].

It looks better now, but still I am not sure I like the result. I now have a chance to do an isSstError Check, but it is way more complicated than catching an exception. So what I'm doing is checking the result and signaling it in an exception in my caller code ;-) Not effective, and surely not elegant, but this way the code fits better into the overall picture...

I guess there's no way of getting you to comment on my mid-term wish (replacing error checks with exceptions). It would both make writing new code much easier and also break lots of existing code (maybe I've just answered my question myself... ;-) ). But maybe I am even unaware of any advantages of isXxxError checks throughout some of VAST's frameworks...

Joachim

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/PCg0nSXc2xgJ.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.

John O'Keefe-3

Re: SstSmtpAddress class: bad exception handling

Joachim -

Well, it seems like the problem is a little bigger than just this one method (although that method does need changing).

The SstError is propogated back until it gets to SstSmtpAssembler>>#buildSenderFor: where it will try to use the address (really an SstError) to create a URL. It will fail because an SstError DNU #sstAsUrlComponent. I guess I will open a separate case for this so at least SstSmtpAddress class>>setHost:port:locally:for: can get fixed in V8.5.2.

SST is an 'odd duck' since it uses both SstError and exceptions to handle problems (look for senders of SstError>>raise and raise:). The strategy seems to be to propogate the SstError for a while and then raise an exception. I will need to study this code a little more to figure out the right thing to do.

John

On Sunday, June 3, 2012 1:59:06 AM UTC-4, [hidden email] wrote:

Hi John,

I look forward to seeing 8.5.2.

Do you see any chance for such fundamental changes as getting rid of isXxxError checking any time soon? I know there's going to be a lot of code broken with such changes, but a mixture of exception handling and return value checking makes code harder to maintain and thus systems more brittle. Maybe a first step could be that "public APIs" of frameworks like CFS or Sst throw exceptions...

My guess would be that those checks stem from the way we were taught to program in C for Systems like GEM back in the eighties...

Joachim

Am Mittwoch, 30. Mai 2012 19:51:21 UTC+2 schrieb John O'Keefe:
Joachim -

What you tried is quite similar to what we will implement in V8.5.2.

The thought process behind using isXxxError instead of exceptions is lost in the mists of time (way before my time). Once the pattern was set in the low level frameworks (like CFS), it was used in most later frameworks.

John

On Friday, May 25, 2012 5:09:32 PM UTC-4, [hidden email] wrote:
Thanks, John.

I tried adding something like this:

host := SstTcpLightTransport officialHostNameFor: sockAddr address.
host isSciError ifTrue: [^SstError for: SstInvalidAddressError with: sockAddr address]].

It looks better now, but still I am not sure I like the result. I now have a chance to do an isSstError Check, but it is way more complicated than catching an exception. So what I'm doing is checking the result and signaling it in an exception in my caller code ;-) Not effective, and surely not elegant, but this way the code fits better into the overall picture...

I guess there's no way of getting you to comment on my mid-term wish (replacing error checks with exceptions). It would both make writing new code much easier and also break lots of existing code (maybe I've just answered my question myself... ;-) ). But maybe I am even unaware of any advantages of isXxxError checks throughout some of VAST's frameworks...

Joachim

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/6Bs9w6UnJ6wJ.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.

jtuchel

Re: SstSmtpAddress class: bad exception handling

John,

thanks for looking into this.

This "propogate the SstError for a while and then raise an exception" makes code hard to read and SST a beast to use in some cases. Looking into this, you may understand a bit better why I keep complaining about Exception Handling (or better: the absence thereof) in some of VASTs frameworks. There's quite some work to do in case of errors and to understand the error situation and give some useful feedback to the user.

Exceptions have a very good characteristic: they continue to bubble up to my code as long as nobody handles them at will. So if no layer within SST handles it, I can be sure I receive it. It's much easier to code, because there's no way a piece of framework code can "forget" an exception like it happens in SstSmtpAddress class>>setHost:port:locally:for:. If some method somewhere deep down in Sst (or CFS or whatever framework) goes wrong, there's not a battery of isXxxError methods and if statements, just a simple Exception. There's lots of good points in that:

* Your framework code is easier to read and therefor maintain and extend (benefits for both Instantiations and users). I wouldn't be surprised if the number of lines that could be trashed goes above 1000.

* Extensions of SST become easier and cheaper and likely faster to market (ESMTP support with passwords... anyone ? ).

* If an exception occurs (and it has a good error description), the user and the developer have a good chance to see what's wrong

* The debugger will pop up at the correct place, not in some probably (seemingly) completely unrelated place (remember my complains about Log4S and failed Appender creation?)

* Developers can concentrate on handling exceptions, not checking return values and see if there may be some error conditions that need extra care.

* In SST the cycles saved by bubbling up exceptions instead of lots of isXxxError may make SST significantly faster

You know all this, of course. The hardest part of all that is not breaking existing code and finding the resources to change Sst... code...

Nevertheless, I am glad you took the time to understand my problem in its full context, and not only fixing this one method.

And, I must add: Sst is very stable and apart from its "special" coding style proves to be a good base for Seaside and RESTful APIs.

Joachim

Am Donnerstag, 7. Juni 2012 16:28:59 UTC+2 schrieb John O'Keefe:

Joachim -

Well, it seems like the problem is a little bigger than just this one method (although that method does need changing).

The SstError is propogated back until it gets to SstSmtpAssembler>>#buildSenderFor: where it will try to use the address (really an SstError) to create a URL. It will fail because an SstError DNU #sstAsUrlComponent. I guess I will open a separate case for this so at least SstSmtpAddress class>>setHost:port:locally:for: can get fixed in V8.5.2.

SST is an 'odd duck' since it uses both SstError and exceptions to handle problems (look for senders of SstError>>raise and raise:). The strategy seems to be to propogate the SstError for a while and then raise an exception. I will need to study this code a little more to figure out the right thing to do.

John

On Sunday, June 3, 2012 1:59:06 AM UTC-4, [hidden email] wrote:
Hi John,

I look forward to seeing 8.5.2.

Do you see any chance for such fundamental changes as getting rid of isXxxError checking any time soon? I know there's going to be a lot of code broken with such changes, but a mixture of exception handling and return value checking makes code harder to maintain and thus systems more brittle. Maybe a first step could be that "public APIs" of frameworks like CFS or Sst throw exceptions...

My guess would be that those checks stem from the way we were taught to program in C for Systems like GEM back in the eighties...

Joachim

Am Mittwoch, 30. Mai 2012 19:51:21 UTC+2 schrieb John O'Keefe:
Joachim -

What you tried is quite similar to what we will implement in V8.5.2.

The thought process behind using isXxxError instead of exceptions is lost in the mists of time (way before my time). Once the pattern was set in the low level frameworks (like CFS), it was used in most later frameworks.

John

On Friday, May 25, 2012 5:09:32 PM UTC-4, [hidden email] wrote:
Thanks, John.

I tried adding something like this:

host := SstTcpLightTransport officialHostNameFor: sockAddr address.
host isSciError ifTrue: [^SstError for: SstInvalidAddressError with: sockAddr address]].

It looks better now, but still I am not sure I like the result. I now have a chance to do an isSstError Check, but it is way more complicated than catching an exception. So what I'm doing is checking the result and signaling it in an exception in my caller code ;-) Not effective, and surely not elegant, but this way the code fits better into the overall picture...

I guess there's no way of getting you to comment on my mid-term wish (replacing error checks with exceptions). It would both make writing new code much easier and also break lots of existing code (maybe I've just answered my question myself... ;-) ). But maybe I am even unaware of any advantages of isXxxError checks throughout some of VAST's frameworks...

Joachim

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

Am Freitag, 25. Mai 2012 16:37:30 UTC+2 schrieb John O'Keefe:
Joachim -

I've opened Case 49667: Missing error check in SstSmtpAddress class>>setHost:port:locally:for:. A fix is scheduled for V8.5.2.

John

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/AnEiD4B67OQJ.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.

jtuchel

Re: SstSmtpAddress class: bad exception handling

John

one idea that might help get things started: Maybe you could turn SstError into an "empty zombie" class that simply raises an exception, so that all the existing isSstError checks would simply be ignored and could be removed one by one, once regression tests have proven this concept to work.

This could be the start of a prototype for the next generation SST.

Joachim

--
You received this message because you are subscribed to the Google Groups "VA Smalltalk" group.
To view this discussion on the web visit https://groups.google.com/d/msg/va-smalltalk/-/c4lERfi0sh8J.
To post to this group, send email to [hidden email].
To unsubscribe from this group, send email to [hidden email].
For more options, visit this group at http://groups.google.com/group/va-smalltalk?hl=en.