[Pharo-project] Problem with announcements in Seaside when using Pharo 1.3
Hi all, In have a simple case: in an ajax callback I trigger an announcement and in the callback associated with the announcement I try to render a Date object. When I do this I get a WARequestContextNotFound exception. To be more exact I have in a component this function to render the callback link: renderAnnouncementUpdate: html html anchor onClick: (html jQuery ajax script: [ :s | script := s. model announce: SGLLinkClickedAnnouncement new ]); with: 'Update Announcements' The model is initialized when I create the component in: initialize super initialize. model := SGLVerySimplePresentation new. model on: SGLLinkClickedAnnouncement do: [:ann| self onClick: ann]. When the announcement is called it will trigger the onclick: method which will do the rendering: onClick: ann self addScripts: script addScripts: s s << (s jQuery: #date1) html: [ :r | r render: Date today ]. s << (s jQuery: #time1) html: [ :r | r render: Time now ] However the html: method trigers an WARequestContextNotFound exception. I'v tried this on Pharo 1.3 (running on CogVM) with the latest version of Seaside. The application can be found in the Moose Playground project from http://www.squeaksource.com in the package SGL-Seaside-Problem. Atfer you install the package and start a web server you have to execute "SGLTestComponentMin registerAsApplication: 'SGLTestComponentMin'." to register the application. Am I missing something? Cheers, Andrei
Not sure why the request context is suddenly gone in your code. It could be that this has something to do with the announcements, the implementation changed drastically in Pharo 1.3. I had to revert the announcements to the original implementation in OB to make them work. Do you properly receive the request context before doing the announcement, just to rule out any problems in Seaside? Seaside has not really been tested on Pharo 1.3 yet, our builds are all based on stable Pharo 1.2.1. Lukas On 17 July 2011 18:53, Andrei Vasile Chis <chisvasileandrei@gmail.com> wrote:
Hi all,
In have a simple case: in an ajax callback I trigger an announcement and in the callback associated with the announcement I try to render a Date object. When I do this I get a WARequestContextNotFound exception. To be more exact I have in a component this function to render the callback link: renderAnnouncementUpdate: html  html anchor   onClick: (html jQuery ajax script: [ :s | script := s. model announce: SGLLinkClickedAnnouncement new ]);    with: 'Update Announcements' The model is initialized when I create the component in: initialize super initialize. model := SGLVerySimplePresentation new. model on: SGLLinkClickedAnnouncement do: [:ann| self onClick: ann]. When the announcement is called it will trigger the onclick: method which will do the rendering: onClick: ann self addScripts: script addScripts: s s << (s jQuery: #date1)        html: [ :r | r render: Date today ].    s << (s jQuery: #time1)        html: [ :r | r render: Time now ] However the html: method trigers an WARequestContextNotFound exception. I'v tried this on Pharo 1.3 (running on CogVM) with the latest version of Seaside. The application can be found in the Moose Playground project from http://www.squeaksource.com in the package SGL-Seaside-Problem. Atfer you install the package and start a web server you have to execute "SGLTestComponentMin registerAsApplication: 'SGLTestComponentMin'." to register the application. Am I missing something? Cheers, Andrei
_______________________________________________ seaside mailing list seaside@lists.squeakfoundation.org http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
-- Lukas Renggli www.lukas-renggli.ch
Hi lukas in 1.3 we introduced on:fork: to ensure that for example a system announcement is processed even if a user announcement would break for the same registry. The idea is that on Error the stack is copied and forked and the current process is resumed. Now with DynamicVariable (and probably Notification) the stack is cut and the resume of the notification or dynamic variable gets nuts because it does not find the information it needs. We believe that on: fork: will really improve the stability of the system (ui process, announcement). We want to be able to debug UI process without having to rinstall the current process and all the current magic. I do not want to see the new package system on its knees just because somebody registered a broken announcement on a systemEvent (it already happened before on:fork:). Now Igor published a new version of on:fork: that only performs the fork and cuts the stack on exception that are not always resuming. To indicate that an exception is alwaysResuming, it is necessary to define alwaysResuming ^ true and we are done. So we did a try on Glamour Seaside by adding DynamicVariable>>alwaysResuming ^ true and we are open to suggestion because we are thinking about putting Notification >>alwaysResuming ^ true So for Seaside it would be good that you introduce such method. Stef On Jul 17, 2011, at 7:17 PM, Lukas Renggli wrote:
Not sure why the request context is suddenly gone in your code.
It could be that this has something to do with the announcements, the implementation changed drastically in Pharo 1.3. I had to revert the announcements to the original implementation in OB to make them work.
Do you properly receive the request context before doing the announcement, just to rule out any problems in Seaside? Seaside has not really been tested on Pharo 1.3 yet, our builds are all based on stable Pharo 1.2.1.
Lukas
On 17 July 2011 18:53, Andrei Vasile Chis <chisvasileandrei@gmail.com> wrote:
Hi all,
In have a simple case: in an ajax callback I trigger an announcement and in the callback associated with the announcement I try to render a Date object. When I do this I get a WARequestContextNotFound exception. To be more exact I have in a component this function to render the callback link: renderAnnouncementUpdate: html html anchor onClick: (html jQuery ajax script: [ :s | script := s. model announce: SGLLinkClickedAnnouncement new ]); with: 'Update Announcements' The model is initialized when I create the component in: initialize super initialize. model := SGLVerySimplePresentation new. model on: SGLLinkClickedAnnouncement do: [:ann| self onClick: ann]. When the announcement is called it will trigger the onclick: method which will do the rendering: onClick: ann self addScripts: script addScripts: s s << (s jQuery: #date1) html: [ :r | r render: Date today ]. s << (s jQuery: #time1) html: [ :r | r render: Time now ] However the html: method trigers an WARequestContextNotFound exception. I'v tried this on Pharo 1.3 (running on CogVM) with the latest version of Seaside. The application can be found in the Moose Playground project from http://www.squeaksource.com in the package SGL-Seaside-Problem. Atfer you install the package and start a web server you have to execute "SGLTestComponentMin registerAsApplication: 'SGLTestComponentMin'." to register the application. Am I missing something? Cheers, Andrei
_______________________________________________ seaside mailing list seaside@lists.squeakfoundation.org http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
-- Lukas Renggli www.lukas-renggli.ch
in 1.3 we introduced on:fork: to ensure that for example a system announcement is processed even if a user announcement would break for the same registry.
I saw that, but didn't understand the implications.
The idea is that on Error the stack is copied and forked and the current process is resumed.
The problem is that the code does not only catches Error, but Exception. This means it will also capture notifications, or any other exception that your code might be handling.
Now with DynamicVariable (and probably Notification) the stack is cut and the resume of the notification or dynamic variable gets nuts because it does not find the information it needs.
I think this is a serious problem in the new announcer framework. Announcing something should not affect the existing exception handing. I believe the problem could be fixed (for most users) if announcements would catch UnhandledError only, this exception is raised to open the debugger.
We believe that on: fork: will really improve the stability of the system (ui process, announcement). We want to be able to debug UI process without having to rinstall the current process and all the current magic. I do not want to see the new package system on its knees just because somebody registered a broken announcement on a systemEvent (it already happened before on:fork:).
Yes, but you should not interfere with intentionally raised exceptions (and this is not only for notifications). The current announcement implementation breaks many users and many legitimate use-cases. Lukas -- Lukas Renggli www.lukas-renggli.ch
We will think hard and see what is a reasonable solution. What we want is the garantee that announcements are processed, that UI continue to work even if a piece of code break part of it, same for event handling.
in 1.3 we introduced on:fork: to ensure that for example a system announcement is processed even if a user announcement would break for the same registry.
I saw that, but didn't understand the implications.
The idea is that on Error the stack is copied and forked and the current process is resumed.
The problem is that the code does not only catches Error, but Exception. This means it will also capture notifications, or any other exception that your code might be handling.
Indeed this is why we probably need to way to let the programmer define the exception that he does not want to be caught.
Now with DynamicVariable (and probably Notification) the stack is cut and the resume of the notification or dynamic variable gets nuts because it does not find the information it needs.
I think this is a serious problem in the new announcer framework.
From a certain perspective it also work better than the previous one, so this is not black and white.
Announcing something should not affect the existing exception handing.
Yes but errors in handler should not break other announcements.
I believe the problem could be fixed (for most users) if announcements would catch UnhandledError only, this exception is raised to open the debugger.
May be, we will think about that.
We believe that on: fork: will really improve the stability of the system (ui process, announcement). We want to be able to debug UI process without having to rinstall the current process and all the current magic. I do not want to see the new package system on its knees just because somebody registered a broken announcement on a systemEvent (it already happened before on:fork:).
Yes, but you should not interfere with intentionally raised exceptions (and this is not only for notifications). The current announcement implementation breaks many users and many legitimate use-cases.
do you have some examples so that we can understand on real use cases? because we were checking ours and it worked for us. Again there are parts of the system that we want really robust and breaking the propagation of announcement because one is broken is not really good.
Lukas
-- Lukas Renggli www.lukas-renggli.ch
Announcing something should not affect the existing exception handing.
Yes but errors in handler should not break other announcements.
I agree, but then it should be possible to intentionally abort the announcement handling. This behavior is used in OB to warn the user about an unsaved method and potentially abort the announcement processing. But then again, this will never work with the new Pharo announcements, because the handlers are stored in an IdentitySet and are executed in random order anyway.
Yes, but you should not interfere with intentionally raised exceptions (and this is not only for notifications). The current announcement implementation breaks many users and many legitimate use-cases.
do you have some examples so that we can understand on real use cases?
Any application that handles exceptions itself and that uses announcements likely breaks with then changes in Pharo 1.3: - Any UI tool that does interactions and queries through exception processing: OB, users of UIManager, MenuMorph, ... - Any persistency layer that ties transactions to the stack (or process): Magma, OmniBase, Goods, ROE, ... - Any web framework (or other non-morphic UI tool) that has its own exception handling: Seaside, Kom, GTK, Cocoa, ...
Again there are parts of the system that we want really robust and breaking the propagation of announcement because one is broken is not really good.
Now this is not really a big deal for me, OB works perfectly in Pharo 1.3 with its own "old" announcement implementation. It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above. Lukas -- Lukas Renggli www.lukas-renggli.ch
On Jul 17, 2011, at 9:57 PM, Lukas Renggli wrote:
Announcing something should not affect the existing exception handing.
Yes but errors in handler should not break other announcements.
I agree, but then it should be possible to intentionally abort the announcement handling.
This behavior is used in OB to warn the user about an unsaved method and potentially abort the announcement processing. But then again, this will never work with the new Pharo announcements, because the handlers are stored in an IdentitySet and are executed in random order anyway.
ok but if you do not say it how can we improve? So now we learned that using an identitySet was a bad idea. You see if you would have given information when you got the problems we would have probably fixed everything.
Yes, but you should not interfere with intentionally raised exceptions (and this is not only for notifications). The current announcement implementation breaks many users and many legitimate use-cases.
do you have some examples so that we can understand on real use cases?
Any application that handles exceptions itself and that uses announcements likely breaks with then changes in Pharo 1.3:
- Any UI tool that does interactions and queries through exception processing: OB, users of UIManager, MenuMorph, ...
OB I know but this is notification, no? UIManager? really for the progressNotification? MenuMorph is using Exception?
- Any persistency layer that ties transactions to the stack (or process): Magma, OmniBase, Goods, ROE, ... - Any web framework (or other non-morphic UI tool) that has its own exception handling: Seaside, Kom, GTK, Cocoa, ...
Again there are parts of the system that we want really robust and breaking the propagation of announcement because one is broken is not really good.
Now this is not really a big deal for me, OB works perfectly in Pharo 1.3 with its own "old" announcement implementation.
this is not the point. We want to make the infrastructure really good. So we will fix that but for that we need inputs. If everybody "fixes" his stuff in his corner then we should better stop pharo.
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower. Stef
Lukas
-- Lukas Renggli www.lukas-renggli.ch
This behavior is used in OB to warn the user about an unsaved method and potentially abort the announcement processing. But then again, this will never work with the new Pharo announcements, because the handlers are stored in an IdentitySet and are executed in random order anyway.
ok but if you do not say it how can we improve? So now we learned that using an identitySet was a bad idea. You see if you would have given information when you got the problems we would have probably fixed everything.
The new implementation is so complicated, I only start to understand all implications now. People were telling me how cool and how important the new implementation is, so I thought I better take my own 3-method/2-class implementation that does exactly what was needed.
- Any UI tool that does interactions and queries through exception processing: OB, users of UIManager, MenuMorph, ...
OB I know but this is notification, no?
Yes, these are notifications.
UIManager? really for the progressNotification?
ProvideAnswerNotification. And yes, there is also ProgressInitiationException for the progress bar, which is not a Notification.
MenuMorph is using Exception?
Yes. Just look at the subclasses of Exception and ask yourself which one people are commonly handling. ZeroDivide, FileDoesNotExistException, MessageNotUnderstood, ConnectionTimedOut, ProgressInitiationException, ... are all Exceptions, some of them are resumable.
Now this is not really a big deal for me, OB works perfectly in Pharo 1.3 with its own "old" announcement implementation.
this is not the point. We want to make the infrastructure really good. So we will fix that but for that we need inputs. If everybody "fixes" his stuff in his corner then we should better stop pharo.
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower.
IMHO, the goal of Pharo should be to provide a minimal Smalltalk system where people Pharo can load what they need. Instead Pharo keeps on growing. I would prefer a system where I can decide myself what libraries and tools I want to use. I am sure that the current Announcement implementation can be fixed, but does it really make sense that the core team bothers about something that could be external? Lukas -- Lukas Renggli www.lukas-renggli.ch
Hi, On 17 Jul 2011, at 22:35, Lukas Renggli wrote:
This behavior is used in OB to warn the user about an unsaved method and potentially abort the announcement processing. But then again, this will never work with the new Pharo announcements, because the handlers are stored in an IdentitySet and are executed in random order anyway.
ok but if you do not say it how can we improve? So now we learned that using an identitySet was a bad idea. You see if you would have given information when you got the problems we would have probably fixed everything.
The new implementation is so complicated, I only start to understand all implications now. People were telling me how cool and how important the new implementation is, so I thought I better take my own 3-method/2-class implementation that does exactly what was needed.
- Any UI tool that does interactions and queries through exception processing: OB, users of UIManager, MenuMorph, ...
OB I know but this is notification, no?
Yes, these are notifications.
UIManager? really for the progressNotification?
ProvideAnswerNotification.
And yes, there is also ProgressInitiationException for the progress bar, which is not a Notification.
MenuMorph is using Exception?
Yes.
Just look at the subclasses of Exception and ask yourself which one people are commonly handling. ZeroDivide, FileDoesNotExistException, MessageNotUnderstood, ConnectionTimedOut, ProgressInitiationException, ... are all Exceptions, some of them are resumable.
Now this is not really a big deal for me, OB works perfectly in Pharo 1.3 with its own "old" announcement implementation.
this is not the point. We want to make the infrastructure really good. So we will fix that but for that we need inputs. If everybody "fixes" his stuff in his corner then we should better stop pharo.
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower.
IMHO, the goal of Pharo should be to provide a minimal Smalltalk system where people Pharo can load what they need. Instead Pharo keeps on growing. I would prefer a system where I can decide myself what libraries and tools I want to use. I am sure that the current Announcement implementation can be fixed, but does it really make sense that the core team bothers about something that could be external?
I think it makes a lot of sense. I like very much what the core team is doing. I really do not see how Pharo is growing. I think it is getting more solid, and more modular every day. Yes, there are bumps along the way, but this is why feedback is so important. The Announcement system is very much internal because it is supposed to be used by the core to announce when something changes, and by the IDE to refresh. And yes, the IDE should should be in the feedback loop of the Core development, because it is a critical component. Cheers, Doru
Lukas
-- Lukas Renggli www.lukas-renggli.ch
-- www.tudorgirba.com "Value is always contextual."
IMHO, the goal of Pharo should be to provide a minimal Smalltalk system where people Pharo can load what they need. Instead Pharo keeps on growing. I would prefer a system where I can decide myself what libraries and tools I want to use I am sure that the current Announcement implementation can be fixed, but does it really make sense that the core team bothers about something that could be external?
How can we remove systemNotifier? How can we remove the update/change mechanism? I think that this kind of feedback is not really nice to hear from you may be you should pay attention not to hurt people that I supposed were close to you and perhaps friends. But may be not after all. Let me repeat it again our goal is really to make a minimal core but not more minimal than useless. For me having a Announcement framework that does not work for RPackage, Moose, Glamour, ... is not good. Stef
Let me put a last comment before going to bed. I think that you are unfair because - if you had a look at the list of features in 1.3 you will see that we removed a lot of bad code, - then if you look at the data that marcus sent recently, you will see that the image size did not change that much and that most of the space is taken by monticello timestamp, - finally I do not buy this growing argument because such argument already killed us when we were doing squeak. I prefer 5 nice little classes with a superclass than a fat one. It is easy to judge other work, but I can tell you that we are concerned about size and quality much more than average people. I'm mainly doing pharo on my free time and I'm spending a lot of energy in making it the best system I dream about. Now when is the last time that you said to us: guys you are doing a good job? Have a look at your emails and you will be surprised. Sometimes positive feedback is much better than absence of negative ones. Stef
IMHO, the goal of Pharo should be to provide a minimal Smalltalk system where people Pharo can load what they need. Instead Pharo keeps on growing. I would prefer a system where I can decide myself what libraries and tools I want to use. I am sure that the current Announcement implementation can be fixed, but does it really make sense that the core team bothers about something that could be external?
Lukas
-- Lukas Renggli www.lukas-renggli.ch
It is easy to judge other work, but I can tell you that we are concerned about size and quality much more than average people. I'm mainly doing pharo on my free time and I'm spending a lot of energy in making it the best system I dream about.
Now when is the last time that you said to us: guys you are doing a good job? Have a look at your emails and you will be surprised. Sometimes positive feedback is much better than absence of negative ones.
I did not judge your work. I only pointed out that if new things are integrated (like announcements) it might break with existing users (and how, in this particular case). I am fine if this is the perfect implementation for Moose, Glamour and Pharo itself; you just cannot expect that it is perfect for everybody. There are always users that need something else (in this case I suspect many); and it would be cool if that could be facilitated. Lukas -- Lukas Renggli www.lukas-renggli.ch
I am sure that improving the system is on everyone's intention. Now, let's go back to the problem and see what we can do about it :). Cheers, Doru On 17 Jul 2011, at 23:45, Lukas Renggli wrote:
It is easy to judge other work, but I can tell you that we are concerned about size and quality much more than average people. I'm mainly doing pharo on my free time and I'm spending a lot of energy in making it the best system I dream about.
Now when is the last time that you said to us: guys you are doing a good job? Have a look at your emails and you will be surprised. Sometimes positive feedback is much better than absence of negative ones.
I did not judge your work. I only pointed out that if new things are integrated (like announcements) it might break with existing users (and how, in this particular case). I am fine if this is the perfect implementation for Moose, Glamour and Pharo itself; you just cannot expect that it is perfect for everybody. There are always users that need something else (in this case I suspect many); and it would be cool if that could be facilitated.
Lukas
-- Lukas Renggli www.lukas-renggli.ch
-- www.tudorgirba.com "To lead is not to demand things, it is to make them happen."
On 17 July 2011 22:35, Lukas Renggli <renggli@gmail.com> wrote:
IMHO, the goal of Pharo should be to provide a minimal Smalltalk system where people Pharo can load what they need. Instead Pharo keeps on growing. I would prefer a system where I can decide myself what libraries and tools I want to use. I am sure that the current Announcement implementation can be fixed, but does it really make sense that the core team bothers about something that could be external?
No, Lukas. Announcements is not external and not about growing the system. It is still about shrinking it. As you may know, historically there's a lot of different event delivery mechanisms in image. Some of them: Object>>changed SystemChangeNotifier Morphic ActionSequence/WeakActionSequence and i am sure i missed couple more. And we're planning to replace all of them with single but solid Announcement framework. So, it will clean a lot of places and instead will shrink system considerably. That's why we wanting to have a solid foundation - a robust announcements framework.
Lukas
-- Lukas Renggli www.lukas-renggli.ch
-- Best regards, Igor Stasenko AKA sig.
Am 17.07.2011 um 22:06 schrieb Stéphane Ducasse:
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower.
Why is the usage of on:fork: not an option of the Announcer? We don't have one single Announcer in the system so I don't see the need why they should behave all the same. Over time I think there will be multiple announcers in the system that have different impact in case something goes wrong. For the core announcers on:fork: is probably a good idea to preserve consistency in the system. For the rest it may be better to preserve stack to be able to debug and to reduce side effects. I think on:fork is targetted to an edge case so it should not be the default. my 2 cents, Norbert
Hi, I am having the exact same problem with an Iliad app, which stopped working in 1.3 because it loses the context when catching an announcement. I'd really love to know the solution to this, in the Iliad list we didn't manage to fix it. Cheers, 2011/7/18 Norbert Hartl <norbert@hartl.name>
Am 17.07.2011 um 22:06 schrieb Stéphane Ducasse:
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower.
Why is the usage of on:fork: not an option of the Announcer? We don't have one single Announcer in the system so I don't see the need why they should behave all the same. Over time I think there will be multiple announcers in the system that have different impact in case something goes wrong. For the core announcers on:fork: is probably a good idea to preserve consistency in the system. For the rest it may be better to preserve stack to be able to debug and to reduce side effects. I think on:fork is targetted to an edge case so it should not be the default.
my 2 cents,
Norbert
-- Bernat Romagosa.
Hi Bernat, Until Pharo sorts our how they are going to fix it, we fixed it by overriding two methods. Also see this emial to the Pharo mailing list (that went nowhere): http://lists.gforge.inria.fr/pipermail/pharo-project/2011-July/051094.html AnnouncementSubscription>>deliver: anAnnouncement " Overridden code has a fork in here. Not good." ^ (self handles: anAnnouncement class) ifTrue: [ action cull: anAnnouncement cull: announcer ] SubscriptionRegistry>>deliver: anAnnouncement to: subs subs do: [ :each | each deliver: anAnnouncement ] On Mon, Jul 18, 2011 at 11:13 AM, Bernat Romagosa < tibabenfortlapalanca@gmail.com> wrote:
Hi,
I am having the exact same problem with an Iliad app, which stopped working in 1.3 because it loses the context when catching an announcement. I'd really love to know the solution to this, in the Iliad list we didn't manage to fix it.
Cheers,
2011/7/18 Norbert Hartl <norbert@hartl.name>
Am 17.07.2011 um 22:06 schrieb Stéphane Ducasse:
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower.
Why is the usage of on:fork: not an option of the Announcer? We don't have one single Announcer in the system so I don't see the need why they should behave all the same. Over time I think there will be multiple announcers in the system that have different impact in case something goes wrong. For the core announcers on:fork: is probably a good idea to preserve consistency in the system. For the rest it may be better to preserve stack to be able to debug and to reduce side effects. I think on:fork is targetted to an edge case so it should not be the default.
my 2 cents,
Norbert
-- Bernat Romagosa.
-- Danie Roux *shuffle* Adore Unix - http://danieroux.com Email / Google Talk / Jabber - danie@danieroux.com
I think that the solution is to not catch exception but Unhandled ones. Stef On Jul 18, 2011, at 11:13 AM, Bernat Romagosa wrote:
Hi,
I am having the exact same problem with an Iliad app, which stopped working in 1.3 because it loses the context when catching an announcement. I'd really love to know the solution to this, in the Iliad list we didn't manage to fix it.
Cheers,
2011/7/18 Norbert Hartl <norbert@hartl.name>
Am 17.07.2011 um 22:06 schrieb Stéphane Ducasse:
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower.
Why is the usage of on:fork: not an option of the Announcer? We don't have one single Announcer in the system so I don't see the need why they should behave all the same. Over time I think there will be multiple announcers in the system that have different impact in case something goes wrong. For the core announcers on:fork: is probably a good idea to preserve consistency in the system. For the rest it may be better to preserve stack to be able to debug and to reduce side effects. I think on:fork is targetted to an edge case so it should not be the default.
my 2 cents,
Norbert
-- Bernat Romagosa.
On 17 July 2011 22:06, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote:
On Jul 17, 2011, at 9:57 PM, Lukas Renggli wrote:
Announcing something should not affect the existing exception handing.
Yes but errors in handler should not break other announcements.
I agree, but then it should be possible to intentionally abort the announcement handling.
This behavior is used in OB to warn the user about an unsaved method and potentially abort the announcement processing. But then again, this will never work with the new Pharo announcements, because the handlers are stored in an IdentitySet and are executed in random order anyway.
ok but if you do not say it how can we improve? So now we learned that using an identitySet was a bad idea. You see if you would have given information when you got the problems we would have probably fixed everything.
No, i don't think its bad idea. A different subscriptions may not know about each other (otherwise, why you subscribing twice for same announcement, if you can do it only once?). And in such setup the announcement framework is there to provide a guarantees that announcement will be delivered to subscriber even if one/some of them fail to handle it properly and throw an exception.
From announcer point of view a general expectation that different subscriptions may not know about each other. (i think this is not questionable statement?)
So, if you follow this expectation, it makes no sense to reason which subscriber should receive an announcement first and which one second. Because announcer simply broadcasting announcement to all interesting parties (subscriptions), and there is no need to maintain the order, but instead we should maintain the guarantee that announcement will be delivered to all subscriptions. If you look at this from given angle, you will see that instead, enforcing a specific order is bad idea.
Yes, but you should not interfere with intentionally raised exceptions (and this is not only for notifications). The current announcement implementation breaks many users and many legitimate use-cases.
do you have some examples so that we can understand on real use cases?
Any application that handles exceptions itself and that uses announcements likely breaks with then changes in Pharo 1.3:
- Any UI tool that does interactions and queries through exception processing: OB, users of UIManager, MenuMorph, ...
OB I know but this is notification, no? UIManager? really for the progressNotification? MenuMorph is using Exception?
- Any persistency layer that ties transactions to the stack (or process): Magma, OmniBase, Goods, ROE, ... - Any web framework (or other non-morphic UI tool) that has its own exception handling: Seaside, Kom, GTK, Cocoa, ...
Again there are parts of the system that we want really robust and breaking the propagation of announcement because one is broken is not really good.
Now this is not really a big deal for me, OB works perfectly in Pharo 1.3 with its own "old" announcement implementation.
this is not the point. We want to make the infrastructure really good. So we will fix that but for that we need inputs. If everybody "fixes" his stuff in his corner then we should better stop pharo.
It is just that everybody should be aware of the problem and have its own "simple" announcements when using any of the frameworks above.
you know that this is not the good solution. So probably it was a bad idea to catch everything and probably UnhandledError is a better choice. Now again without feedback we will just progress but much slower.
But where the guarantees that other Exceptions resuming normally and not breaking a normal processing flow? And where the guarantees that all exception handlers sending #resume after done handling them? The problem that here we have conflict of interests: - we want that announcements to be always delivered to all subscribers - but one of the subscribers while handling anouncement may break the processing using Exception(s) and prevent others from handling an announcement. So, to peacefully resolve the conflict, we added #alwaysResuming flag to exception protocol. It should mean that: - an exception is _always_ resuming a normal processing flow after being handled. Note that it is more responsibility of implementors of exception handlers to maintain that property, rather than exception itself. In this case, only if you guarantee that exception won't break a normal processing flow, then on:fork: doesn't forking and not splitting the stack. Otherwise it have to fork (with all consequences) to ensure announcement delivery. Does that sounds reasonable? Also, let me remind you that exceptions are there is to handle an exceptional conditions in processing flow i.e. when something goes wrong and not as expected. So, it is perfectly reasonable to use #on:fork: to prevent announcement handling process from escaping the normal processing flow. Because normally , there should be no exceptions. For instance, in Seaside (if i remember correctly) it creates a separate process for handling request. And so, for dynamic variables you can use a process-specific storage , provided by Process>>environmentAt: and environmentAt:put:, rather than using exceptions to access dynamic variable values. Like that, you don't need to use exceptions and moreover it will work much faster, because unwinding stack each time you accessing the state is costly!
Stef
-- Best regards, Igor Stasenko AKA sig.
You could - however, unlike exception handling, process-local variables are not part of ANSI. Which means it's a safer choice for Seaside from a portability POV. Speaking of which, referring to my last post when Danie raised the issue, is there any reason we don't copy the process environment when forking? Cheers, Henry On 18. juli 2011, at 16:47, Igor Stasenko <siguctua@gmail.com> wrote:
For instance, in Seaside (if i remember correctly) it creates a separate process for handling request. And so, for dynamic variables you can use a process-specific storage , provided by Process>>environmentAt: and environmentAt:put:, rather than using exceptions to access dynamic variable values. Like that, you don't need to use exceptions and moreover it will work much faster, because unwinding stack each time you accessing the state is costly!
On 18 July 2011 19:32, Henrik Johansen <henrik.s.johansen@veloxit.no> wrote:
You could - however, unlike exception handling, process-local variables are not part of ANSI. Which means it's a safer choice for Seaside from a portability POV.
Yes, but seaside using grease layer to deal with dialect-specific stuff. So it is perfectly reasonable to use more effective feature(s) if they are available.
Speaking of which, referring to my last post when Danie raised the issue, is there any reason we don't copy the process environment when forking?
as any feature it needs to have a real use case. Because if its not used by anything, we don't have feedback and it even makes little sense to support it or change it. Without real use case i cannot clearly say, if environment should be inherited when you forking or not. If we draw a parallel with OS process - then yes, in OSes (at least unix ones) they inherit environment and file handles etc , when forking. But if we draw a parallel to threads, the thread-specific storage is clean for new thread. So i really don't know which approach is better without looking at real use case. Unfortunately, we don't have anything which using it now :(
Cheers, Henry
On 18. juli 2011, at 16:47, Igor Stasenko <siguctua@gmail.com> wrote:
For instance, in Seaside (if i remember correctly) it creates a separate process for handling request. And so, for dynamic variables you can use a process-specific storage , provided by Process>>environmentAt: and environmentAt:put:, rather than using exceptions to access dynamic variable values. Like that, you don't need to use exceptions and moreover it will work much faster, because unwinding stack each time you accessing the state is costly!
-- Best regards, Igor Stasenko AKA sig.
Stéphane Ducasse wrote:
We will think hard and see what is a reasonable solution. What we want is the garantee that announcements are processed, that UI continue to work even if a piece of code break part of it, same for event handling.
Lukas suggestion of changing it to fork on UnhandledErrors is a perfectly reasonable solution. In case handlers outside the delivery mechanism itself don't resume, well, the ifCurtailed: block of deliver:to: takes care of delivery to the remaining subscriptions. :) (except there's a bug when asking for after: of last element... ifAbsent: block used _both_ when element not found, AND when there are none after it??) You can still break it though, by using a handler which enters an infinite loop/sleep forever, etc... I agree with Phillipe: "The only way to fix broken code is to fix broken code, not by adding additional hacks.". Robustness is desirable, but only to a certain extent. (IE guard against unforeseen errors, not those handled incorrectly) As for Lukas' concerns not addressed by switching to UnhandledError: In my experience when using announcements in VW, relying on delivery order of announcements isasking for trouble later on... The code paths get hard to follow, maintainability becomes a nightmare as adding new subscribers/modifying old ones might introduce subtle bugs, etc. For the use case you described in OB, why not split it into two announcements instead, the first one vetoable in the sense Vassily wrote about in the blog, and the second announced only if first not vetoed? Cheers, Henry -- View this message in context: http://forum.world.st/Problem-with-announcements-in-Seaside-when-using-Pharo... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
Another problem with the fork not mentioned yet, is the assumption all subscriber actions will have been carried out by the time next statement occurs. fork:'ing breaks that, and you can't make the assumption true without conflicting with the reason to fork: in the first place. Another argument in favor of only forking on unhandled exceptions I guess :) @#after: gripe in previous post: meh, ansi... -- View this message in context: http://forum.world.st/Problem-with-announcements-in-Seaside-when-using-Pharo... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
Yes I have the impression that this is what we should do and do it in 1.3 and 1.4 Stef
Another problem with the fork not mentioned yet, is the assumption all subscriber actions will have been carried out by the time next statement occurs. fork:'ing breaks that, and you can't make the assumption true without conflicting with the reason to fork: in the first place. Another argument in favor of only forking on unhandled exceptions I guess :)
@#after: gripe in previous post: meh, ansi...
-- View this message in context: http://forum.world.st/Problem-with-announcements-in-Seaside-when-using-Pharo... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
On 18 July 2011 13:20, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no> wrote:
Stéphane Ducasse wrote:
We will think hard and see what is a reasonable solution. What we want is the garantee that announcements are processed, that UI continue to work even if a piece of code break part of it, same for event handling.
Lukas suggestion of changing it to fork on UnhandledErrors is a perfectly reasonable solution.
In case handlers outside the delivery mechanism itself don't resume, well, the ifCurtailed: block of deliver:to: takes care of delivery to the remaining subscriptions. :) (except there's a bug when asking for after: of last element... ifAbsent: block used _both_ when element not found, AND when there are none after it??)
Ah, yes. We have ifCurtailed: there as well. So, putting UnhandledError in delivery sounds like good way to resolve the conflict of interests.
You can still break it though, by using a handler which enters an infinite loop/sleep forever, etc... I agree with Phillipe: "The only way to fix broken code is to fix broken code, not by adding additional hacks.". Robustness is desirable, but only to a certain extent. (IE guard against unforeseen errors, not those handled incorrectly)
As for Lukas' concerns not addressed by switching to UnhandledError: In my experience when using announcements in VW, relying on delivery order of announcements isasking for trouble later on... The code paths get hard to follow, maintainability becomes a nightmare as adding new subscribers/modifying old ones might introduce subtle bugs, etc. For the use case you described in OB, why not split it into two announcements instead, the first one vetoable in the sense Vassily wrote about in the blog, and the second announced only if first not vetoed?
indeed.
Cheers, Henry
-- View this message in context: http://forum.world.st/Problem-with-announcements-in-Seaside-when-using-Pharo... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
-- Best regards, Igor Stasenko AKA sig.
Lukas suggestion of changing it to fork on UnhandledErrors is a perfectly reasonable solution.
In case handlers outside the delivery mechanism itself don't resume, well, the ifCurtailed: block of deliver:to: takes care of delivery to the remaining subscriptions. :) (except there's a bug when asking for after: of last element... ifAbsent: block used _both_ when element not found, AND when there are none after it??)
Ah, yes. We have ifCurtailed: there as well. So, putting UnhandledError in delivery sounds like good way to resolve the conflict of interests.
So I let you propose a fix and I will integrate it because I like to do idiot actions :)
You can still break it though, by using a handler which enters an infinite loop/sleep forever, etc... I agree with Phillipe: "The only way to fix broken code is to fix broken code, not by adding additional hacks.". Robustness is desirable, but only to a certain extent. (IE guard against unforeseen errors, not those handled incorrectly)
As for Lukas' concerns not addressed by switching to UnhandledError: In my experience when using announcements in VW, relying on delivery order of announcements isasking for trouble later on... The code paths get hard to follow, maintainability becomes a nightmare as adding new subscribers/modifying old ones might introduce subtle bugs, etc. For the use case you described in OB, why not split it into two announcements instead, the first one vetoable in the sense Vassily wrote about in the blog, and the second announced only if first not vetoed?
indeed.
Ok. Here the fix. I don't like an idea to introduce #alwaysResuming because it brittle. Catching only UnhandledError seems like a good plan. (other stuff will be catched by ifCurtailed: anyways, so we're safe). -- Best regards, Igor Stasenko AKA sig.
On Jul 18, 2011, at 5:11 PM, Igor Stasenko wrote:
Ok. Here the fix.
I don't like an idea to introduce #alwaysResuming because it brittle. Catching only UnhandledError seems like a good plan. (other stuff will be catched by ifCurtailed: anyways, so we're safe).
I agree. I will integrate it now. Stef
On 07/17/2011 08:26 PM, Stéphane Ducasse wrote:
Hi lukas
in 1.3 we introduced on:fork: to ensure that for example a system announcement is processed even if a user announcement would break for the same registry.
Every time you try to be clever you introduce new problems, so don't try to be clever. The only way to fix broken code is to fix broken code, not by adding additional hacks. Cheers Philippe
On Mon, Jul 18, 2011 at 9:01 AM, Philippe Marschall <philippe.marschall@netcetera.ch> wrote:
The only way to fix broken code is to fix broken code, not by adding additional hacks.
I do appreciate all the efforts of the development team. Please go on, try new ideas, and if they don't work you will still have time to revert them. -- Damien Cassou http://damiencassou.seasidehosting.st "Lambdas are relegated to relative obscurity until Java makes them popular by not having them." James Iry
participants (12)
-
Andrei Vasile Chis -
Bernat Romagosa -
Damien Cassou -
Danie Roux -
Henrik Johansen -
Henrik Sperre Johansen
-
Igor Stasenko -
Lukas Renggli -
Norbert Hartl -
Philippe Marschall -
Stéphane Ducasse -
Tudor Girba