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

fix:Fix error when adding two or more publishers #1507

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

zolmk
Copy link

@zolmk zolmk commented Nov 2, 2023

Changes proposed in this pull request:

  • Create a new publisher instance for each event instead of repeatedly calling the initialization method on the same instance. Repeated calls to the initialization of the same instance will result in java.lang.IllegalThreadStateException.

Because there is currently only one publisher for the project, this problem does not occur, and if you register more than one publisher, there will be a problem.

@magestacks
Copy link
Member

@zolmk 1. Submit logs in English

  1. Provide clearer context and how to reproduce the problem

@zolmk
Copy link
Author

zolmk commented Nov 2, 2023

Thanks for your response.

Here are the steps to reproduce the bug:

  1. create a event class, like this
image
  1. Create a service to use this event class, like LongPollingService.
image
  1. We now have an event publisher and event subscriber registered for this event.
  2. Start cn.hippo4j.server.ServerApplication. The following error message will appear:
image

Reason

The reasons for this problem is very simple, when we registered the second event publisher within OtherService constructor, call the NotifyCenter. RegisterToPublisher method, this method has the following code block:

final String topic = ClassUtil.getCanonicalName(eventType);
synchronized (NotifyCenter.class) {
    MapUtil.computeIfAbsent(INSTANCE.publisherMap, topic, publisherFactory, eventType, queueMaxSize);
}

When an event is first registered, the publisherFactory is called to create the event publisher. The publisherFactory code is as follows:

publisherFactory = (cls, buffer) -> {
    try {
        EventPublisher publisher = eventPublisher;
        publisher.init(cls, buffer);
        return publisher;
    } catch (Throwable ex) {
        log.error("Service class newInstance has error: {}", ex);
        throw new RuntimeException(ex);
    }
};

eventPublisher here is the static variable of NotifyCenter, which is already used when the first publisher is registered. So calling the publisher.init method the second time will produce an error, so let's look at some of the code for publisher.init method:

@Override
public void init(Class<? extends AbstractEvent> type, int bufferSize) {
    setDaemon(true);
    setName("dynamic.thread-pool.publisher-" + type.getName());
    this.queueMaxSize = bufferSize;
    this.queue = new ArrayBlockingQueue(bufferSize);
    start();
}

This is where the problem occurs. Calling setDaemon methods multiple times on a thread object results in an error.

@zolmk
Copy link
Author

zolmk commented Nov 2, 2023

@magestacks

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (265bf6e) 34.23% compared to head (ac7d69a) 34.23%.
Report is 4 commits behind head on develop.

❗ Current head ac7d69a differs from pull request most recent head 9c1f2f6. Consider uploading reports for the commit 9c1f2f6 to get more accurate results

Files Patch % Lines
...in/java/cn/hippo4j/config/notify/NotifyCenter.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #1507   +/-   ##
==========================================
  Coverage      34.23%   34.23%           
  Complexity       847      847           
==========================================
  Files            262      262           
  Lines           5953     5953           
  Branches         560      560           
==========================================
  Hits            2038     2038           
  Misses          3722     3722           
  Partials         193      193           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

2 participants