Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Silent failure on StackOverflowException #916

Closed
devrandom opened this issue Oct 23, 2017 · 3 comments
Closed

Silent failure on StackOverflowException #916

devrandom opened this issue Oct 23, 2017 · 3 comments
Assignees
Labels
type/bug A general bug
Milestone

Comments

@devrandom
Copy link

devrandom commented Oct 23, 2017

Expected behavior

Error logged.

Actual behavior

Silent failure and pipeline does not complete.

Steps to reproduce

    @Test
    void testErrorInScheduler() {
        Scheduler scheduler = Schedulers.fromExecutor(Executors.newSingleThreadExecutor(new ThreadFactoryBuilder()
                .setUncaughtExceptionHandler((t, ex) -> System.err.print(ex.toString()))
                .build()));

        Flux<String> source = Flux.just("hi");

        StepVerifier.create(source.flatMap(item -> Mono.<String>fromCallable(() -> {
            throw new StackOverflowError("hi");
        }).log().subscribeOn(scheduler)))
                .expectError()
                .verify();
    }

Reactor Core version

3.0.4-RELEASE

JVM version (e.g. java -version)

1.8

OS version (e.g. uname -a)

Ubuntu 16.04

@smaldini smaldini added this to the 3.1.2.RELEASE milestone Oct 23, 2017
@smaldini smaldini added the type/enhancement A general enhancement label Oct 23, 2017
@smaldini
Copy link
Contributor

We should maybe remove throwIfFatal check in Schedulers.handleError, worst case is that a fatal termination will rethrow anyway (OoM etc).

@smaldini smaldini added type/bug A general bug and removed type/enhancement A general enhancement labels Oct 23, 2017
@dfeist
Copy link
Contributor

dfeist commented Jan 10, 2018

@smaldini Rethrowing here would make sense if WorkerTask used UncaughtExceptionHandler but it doesn't so this needs to be done here, and I see no reason at all why any specific, JVM or other error types, should result in UncaughtExceptionHandler not being used, especially given these errors are not propagated via onErrror.

I also am not sure why onHandleErrorHook should not be notified of these fatal exceptions. Given these errors will not be propagated up, there is no way for the user to handle these errors and perform any logging/cleanup otherwise.

simonbasle added a commit that referenced this issue Jan 12, 2018
The fatal exceptions will probably be thrown down the line and they are
still considered irrecoverable, but when they happen within a Thread
from a Scheduler, they should be passed to the handleError hook
as well as to the Thread's uncaughtExceptionHandler if any.

Such a sequence may hang, as the test demonstrates, but the fatal error
has a chance to be logged.
@devrandom
Copy link
Author

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants