Synchronous calls of asynchronous methods causes errors

[Piotrek Koszuliński]

See e.g. #11295. Calling editor.destroy() right after (or before it's completed) CKEDITOR.replace() or editor.setData() causes errors.

That's because developer has to care about calling methods when previous finished. There are callbacks and events which notify developer when action is completed, but it's his duty to find which method can be called when.

However, not every developer understand problems with asynchronous methods and not in every case it's easy to handle this. For example when editor is loaded in one tab of some UI component and user switches between tabs too quickly, destroy() may be called before editor fully initialises.

On the other hand, if editor's methods would take care of that (e.g. destroy() would wait until initialisation finished), then API would start to work unpredictably. Developer would never know if destroy() will be done immediately or if it's going to wait until something (setData, initialisation, etc.) ends. This may be even worse situation than the current one.

We could make even longer step and make all editor's methods asynchronous and e.g. based on promises. Then everything would be fine... if you understand all of this :D.

Therefore, instead of forcing developers to understand some not trivial concepts to do basic stuff we can simply clarify this in docs. But there still will be cases in which it will be hard to handle editor state (like the tabs case).

Opinions needed.

---

More details about the current situation in ​http://dev.ckeditor.com/ticket/11502#comment:8

[Anders Johansson]

In the case of destroy() being called early, or perhaps DOM nodes simply being gone from the document, I think the editor only need to be more forgiving in the asynchronous functions.

It surely isn't expected to work after destroy() is called, and it would be nice if there was no error messages from pending calls that could not be aborted by destroy().

[Piotrek Koszuliński]

That's not a solution. Hiding errors is the last thing API should do.

[Anders Johansson]

What I'm trying to suggest is that it should not be an error to call destroy() at any time, or to add some other way to explicitly abort/ignore pending operations.

My use case here is like your example with tabed UI component, the CKEditor is only a part of a larger application. A lot of stuff can happen between starting the initialization of the editor and the execution of a delayed function coming back from setTimeout(f, 0);

The only workaround I can currently think of would be to move the editor to a hidden corner of the DOM and wait for it to finish whatever it is doing before it is killed. And that might not even work due to problems with the iframe loosing its state.

[Artur Delura]

Me as a new developer had a lot of trouble with this. I called destroy method before editor was fully initialized. First of all, I saw an error in wyswig plugin which tells me nothing. I commented out about 350 lines of my code to discover the problem (destroy method was the last thing I thougt about :) ).

I think that destroy method should handle asynchromous calls which are not done, and interrupt them. Or at least developer should see proper information what is wrong. It could save him a lot of time, and overall feeling about API would be better.

[Piotrek Koszuliński]

DUP reported: #11924.

[Jakub Ś]

#11747 was marked as duplicate.

[Piotrek Koszuliński]

I wrote a pretty detailed post about the current issues and problems with solving them in #12859. Quoting myself... :D

---

Why CKEditor fails when I try to call some methods one after another?

editor.setData( someData );
// somewhere later...
editor.destroy();

The answer, unfortunately, isn't simple. Otherwise we'd have this fixed a long time ago. This is a mix of history, backwards compatibility, architectural limitations and lack of good solutions.

First thing to notice is that this is not only editor.setData's problem, because other editor's methods could malfunction too. Editor.getData, editor.destroy, editor.setReadOnly, etc. are not going to work if editor is not in the ready state. Furthermore, this problem concerns some other classes as well - e.g. CKEDITOR.editable.

Second thing to notice is that this problem occurs not only when editor isn't loaded yet, but also when switching between states and setting data (both operations can be asynchronous). This introduces more variables to the equation and could even make the state checks pretty ugly. Fortunately, we introduced editor.status and editable.status (although, we did this pretty recently), so currently this wouldn't be that bad.

Another architectural aspect that would have to be taken into consideration is how the code is decoupled. For instance, the editor.setData method only fires an event. If we started checking the editable's state inside this method we would create a tight relations and could break some existing implementations.

Backwards compatibility is important for one more reason - we can make backward incompatible changes, but we must do them in a smart way. For instance, once it's done it should be final - we can't ship some other solution in the next major release if this one turns out to be unsatisfactory. This rises the stake pretty high.

The last thing, and actually the most unfortunate, is that throwing errors is not that good solution. It may tell you that you're doing something wrong, but it won't tell you when you can do it.

try {
	editor.setData( ... );
} catch ( err ) {
	if ( err instanceof CKEDITOR.editorNotReadyError ) {
		// apparently, it wasn't the right moment
		// ... and what now? Ok, let's wait for some imaginary "ready" event
		// (which doesn't exist right now):
		editor.once( 'ready', function() {
			editor.setData( ... );
			// ... actually, this could throw again if some other editor#ready
			// listener called setData too and haven't cancel the event.
		} );
	} else {
		throw err;
	}
}

Isn't it ugly? I think it's a terrible developer experience. A more useful approach would to be cache method calls and execute them when the editor reaches the correct state. But this would mean that for instance editor.destroy, which is currently a synchronous method, would become an asynchronous one because it can't be executed while editor is not ready. Moreover, to make all this possible to implement we would need to do serious changes not only in the API, but in the editor internals as well. Even the highly unsatisfactory solution with throwing errors requires significant effort to and may lead to ugly compatibility issues.

To sum up. I don't want say that fixing this in CKEditor 4.x is impossible, but for now we didn't have enough resources and well... courage, to attempt that. I would also risk a statement that it's late in CKEditor 4.x for such changes. But most importantly, we are aware of these problems and will definitely ​design CKEditor 5's API differently.

[Jakub Ś]

#12855 and #10501 were marked as duplicates.

Although these tickets were closed, please have a look at them as they contain some very interesting details.

[Cyril]

I've opened a PR to fix some of those issues ​https://github.com/ckeditor/ckeditor-dev/pull/170

How likely are you to accept work like that?

[Dusty Jewett]

Is there any way we can get this addressed? This issue occurs often when developing Single Page Apps.

I think that it's quite clear that an error should NEVER occur from a callback passed to setTimeout(). I'm fine with an exception bubbling up from destroy(), but this API shouldn't be throwing exceptions that are impossible to catch.

[Piotrek Koszuliński]

Nothing changed since I wrote comment:8. To have this issue completely addressed the project would require serious API changes, because we're talking here about introducing promises and perhaps some other mechanisms throughout a big part of the code base. This is a massive task and massive API change (likely backward incompatible).

Therefore, taking into account that we already started works on CKEditor 5 (see ​https://github.com/ckeditor/ckeditor5-design and ​https://github.com/ckeditor/ckeditor5) I don't see a chance for a real change in CKEditor 4. Some smaller issues can be addressed, especially when supported by 3rd party contributions, but that would be more like curing symptoms than fixing the real issues.

PS. Please remember that in most cases CKEditor API will not throw errors when used correctly. The only problem is that necessary checks (and synchronisation of calls) must be done a developer. There are may be some cases when it's really impossible to tell if e.g. the editor can be destroyed, but most of the time it's doable. Even introducing promises or making the currently thrown errors catchable won't solve your issues, because if an error is thrown it means that the API was already used incorrectly. Don't expect CKEditor to do everything.

[Dusty Jewett]

Here's an example of how things break, despite the API being used 'correctly'. Notice how it is impossible for us to catch the exception.

​http://jsfiddle.net/dustyjewett/2Lpa6pjm/2/

This is a VERY simple/common example of what happens in current SPAs. Let's say that the session has just timed out. What do we do? We immediately clear out whatever data stores we have and re-direct the user to a login screen. Clearing out data might cause the current screen to re-render itself, which is fine... but the redirect, which will likely happen during the same event loop, will remove the entire DOM tree.

I'm fine with catching exceptions and not expecting CKEditor to "Do Everything"... The problem comes from the proliferation of the usage of setTimeout(). Because wysiwygarea and autoresize plugins don't check for DOM existence before attempting to access/modify it, these are the two most common places we see exceptions thrown... but because it's inside of a setTimeout(), we can't catch it. (We've resorted to the very UGLY fix of overriding CKEDITOR.tools.setTimeout with our own method that simply adds try{} and calls our given exception handler.)

There are two pull requests (170 and 200) that are currently attempting to fix the same class of issue. Both have been open for a while (5 months and 24 days, respectively).

It seems reasonable to guard against known possible faults, such as checking for destroyed or dom before continuing with a method that was delayed with setTimeout().

And to be clear, I'm not asking for a re-architecture... just stop throwing exceptions that can't be caught...

[Piotrek Koszuliński]

Here's an example of how things break, despite the API being used 'correctly'. Notice how it is impossible for us to catch the exception.

​​http://jsfiddle.net/dustyjewett/2Lpa6pjm/2/

I wouldn't say it's a correct usage if you unplug a resource that's currently busy (CKEditor while initialisation). You will have the same kind of problems with many devices and applications. And there will never be a cure for this – very few software can deal with cut off resources and we will never try to be one as it a different level of complexity.

Of course, the errors should be catchable. But as I wrote - that requires significant architectural changes. Not doable for CKEditor 4.

One thing I would like to fully understand is why you must catch all errors. The errors CKEditor may throw will not break your application as they are thrown asynchronously. Normally I would expect that you want to catch them to be able to react, but I can't imagine any reaction to autogrow's insignificant timeout. Error thrown there neither affects CKEditor, nor your app.

---

There are two pull requests (170 and 200) that are currently attempting to fix the same class of issue. Both have been open for a while (5 months and 24 days, respectively).

They deal with a very small part of this problem. None of them was also fully complete, so we couldn't just accept them. However, as I commented in ​https://github.com/ckeditor/ckeditor-dev/pull/200#issuecomment-126681990 we're planning to accept this PR after additional research and tests which it requires. We are also willing to accept PRs fixing similar issues in other parts of the code base.

I was opposing similar attempts in the past, but that was because we didn't know how CKEditor 5 will look. Now it's clear that it is a separate project, not a next iteration, so we can find a feasible solution for CKEditor 4 too.

[Dusty Jewett]

Uncaught Exceptions impacts:

1. Tech-Saavy customers will see that there are exceptions in the console. Exceptions are a red flag in security audits, and we have to explain how any that exist aren't potential vulnerabilities.

2. We do catch all uncaught exceptions thrown by the page, eventually. (They still show up in the console, however) We treat every exception as, well, an exception, and try to fix them. We use OpBeat, but NewRelic and Airbrake offer js libraries that ship JS exception logs. An exception thrown in our app is a big deal that we try to fix immediately... The noise created by CKEditor drowns out the actual exceptions that we need to fix.

3. Our tests fail when there are exceptions. It is impossible for us to deterministically know when we can tear down the editor in tests. Autogrow does not set a flag that we can check, so inputting text can trigger autogrow, but we have no way of knowing that is pending. Our only solution is to add TWO setTimeout calls during our teardowns, which has reduced our intermittent CKEditor failures in tests down to 1-in-10.

If I submit a backwards-compatible patch for tools.setTimeout, autogrow and wysiwygarea, will they be reviewed/tested/accepted in a timely manner? Sure, it might not be this grand system that you want to have that will solve all of the problems, make anything synchronous, or ensure that destroy() will succeed... But it'll allow for devs to catch/check/handle exceptions when required.

[Piotrek Koszuliński]

I understand all the reasons of course. Thanks for clarifying it.

So, I would draw this plan:

1. We work on #13632 which I opened today. This will give us a ground for the rest of the changes.
2. We close ​PR#200 and perhaps ​PR#170 too (depends on how complete it is).
3. Then, please report separately tickets for errors that you want to eliminate. Please write a comment in the current ticket, so I'll be notified about the new ones.
4. We will let you know what's the expected way of fixing that issue. This is really case by case thing, so I can't say now what we would make sense and what not.
5. Then, we will review and eventually close your PRs in a timely manner (this means some feedback in ~1 week max; I can't promise more because we have other tasks too).

[Piotrek Koszuliński]

PS. Regarding #13632. It's an important ticket for us, because for other developers it may be important that CKEditor does not throw errors, but for us it's the opposite. The more verbose it is, the better, because we can faster find and issue and debug it. We had many problems in the past with empty catch{} blocks and other ways which were silencing potential console logs. I want to avoid having this problem again because it's a really short term improvement if we just treat symptoms.

[Dusty Jewett]

Any update on this ticket? The branch for #13632 has sat un-touched for two weeks.

This method, specifically, causes me troubles, and I want to ensure that it is properly caught:

​https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/autogrow/plugin.js#L54-L67

Specifically, this line actually throws the exception: ​https://github.com/ckeditor/ckeditor-dev/blob/master/core/dom/element.js#L1450

This happens when the DOM is removed prior to full initialization.

This code doesn't use tools.setTimeout, so I'm not able to override that method to add try/catch blocks around it.

[Guillaume L.]

Replying to dustyj-pn:

And to be clear, I'm not asking for a re-architecture... just stop throwing exceptions that can't be caught...

I have the same issue, I need to prevent exceptions to reach the window (a custom exception handler API would be fine, I don't need CKE to handle every corner case).

@dustyj-pn thanks for the pointer for a workaround, will try that.

For anyone interested: I also use CKE in a SPA and in my case, switching between tabs very fast can cause the destroy before init complete issue and I'd like to be able to ignore that error in that case.

[Jakub Ś]

#8545 was marked as duplicate.