Skip to content
This repository has been archived by the owner on Oct 30, 2022. It is now read-only.

在增删mIResponses的时候遍历通知没有处理清楚 #8

Closed
447zyg opened this issue Feb 13, 2019 · 8 comments
Closed

在增删mIResponses的时候遍历通知没有处理清楚 #8

447zyg opened this issue Feb 13, 2019 · 8 comments

Comments

@447zyg
Copy link

447zyg commented Feb 13, 2019

虽然考虑了用int length = mIResponses.size();
来处理ConcurrentModificationException
但是如果删减了mIResponses
循环就数组越界了。
也可能导致跳过需要接收的观察者

@KunMinX
Copy link
Owner

KunMinX commented Feb 16, 2019

@447zyg 谢谢你的检查和反馈 ^_^

@KunMinX
Copy link
Owner

KunMinX commented Feb 16, 2019

@447zyg 增删响应者的策略已完成改进。你的这个反馈对我们很重要!🙏

@447zyg
Copy link
Author

447zyg commented Feb 18, 2019

@KunMinX hi 我看了修改的代码 但是我觉得 这样的解决翻案并不是最终解决方案。我之前也提过增加synchronized 但是我觉得并不好所以修改了。
现在这个方案存在这个问题

前提 用户网络不好。进入页面加载 但是这时候网不好 退出了(仍在加载中) 又进来又加载一次。
这时候如果上一次的请求有结果了 会返回2次数据 都会显示在页面上(有坑点)

而且这不符合设计理念。
再onDestroy的时候我的监听就应该被移除而不是放在另一个队列
这样仍被持有是否存在内存泄露的问题。
我暂时没想出合理的解决方案

@KunMinX
Copy link
Owner

KunMinX commented Feb 18, 2019

@447zyg 哈哈,你考虑的比较周全哈,我相信实际使用中 多多少少会遇到 你刚刚提到的情况。

确实,由于页面和业务完全分离,导致业务的执行不受页面控制,不与页面共存亡,使得下次哪怕页面注册个新的响应者实例,依然会因为响应码的匹配,而有可能重复接收不够及时发来的消息。

包括后面提到的,内存泄漏的问题,我再想想 😂

@447zyg
Copy link
Author

447zyg commented Feb 18, 2019

@KunMinX 😂内存泄露好像 加了 leakcanary 测试 暂时没发现 因为持有的引用在下次收到数据要发送通知的时候还是会被remove掉。我之前有考虑采用CopyOnWriteArrayList (在你改代码之前) 感觉好像还行?还是也有隐藏的坑。暂时还没发现

@KunMinX
Copy link
Owner

KunMinX commented Feb 19, 2019

@447zyg 啊,能预见“过时消息的发送”,感觉你对并发的处理有一定经验哈,放手做吧,我相信你!

@447zyg
Copy link
Author

447zyg commented Feb 20, 2019

@KunMinX
之前我提问题的时候的想法是基于多线程访问这个BaseBus 如果只有主线程访问
那也就没有我说的那些问题了

关于多线程访问
这是我找到的一种解决方案 通过在 add、remove、以及notify的时候上锁

`public class BaseBus {

private static Set<IResponse> mIResponses;

private static final Object LOCK = new Object();

public static void registerResponseObserver(IResponse response) {
    if (response == null) return;

    synchronized (LOCK) {
        if (mIResponses == null) {
            mIResponses = new HashSet<>(1);
        }
        mIResponses.add(response);
    }
}

public static void unregisterResponseObserver(IResponse response) {
    if (response == null) return;

    synchronized (LOCK) {
        if (mIResponses != null) {
            mIResponses.remove(response);
        }
    }
}

public static void response(Result result) {

    if (result == null) {
        return;
    }

    Set<IResponse> mIResponsesCopy;

    synchronized (LOCK) {
        if (mIResponses == null) {
            return;
        }
        mIResponsesCopy = new HashSet<>(mIResponses);
    }

    for (IResponse response : mIResponsesCopy) {
        response.onResult(result);
    }
    mIResponsesCopy.clear();
    mIResponsesCopy = null;
}

}`

@KunMinX
Copy link
Owner

KunMinX commented Feb 21, 2019

@447zyg

@447zyg 447zyg closed this as completed Feb 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants