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

Can not find room after add member to the room #1380

Closed
windmemory opened this Issue Jun 23, 2018 · 15 comments

Comments

Projects
None yet
2 participants
@windmemory
Copy link
Contributor

windmemory commented Jun 23, 2018

Expected behavior

Able to find the room

Actual behavior

The room can not be found

Investigation

  • In padchat-manager.ts function getRoomIdList is using cacheRoomRawPayload as the datasource to get the room id list.
  • Each time Room.add get called, it will call roomAdd in puppet.ts. The implementation of roomAdd in padchat-puppet.ts will call roomPayloadDirty which will cleanup the room data from cache. Thus result in missing room id in the room list.

Suggestion

It would be better to reload the room information each time we call the roomPayloadDirty. And I think it makes sense to directly change the function roomPayloadDirty to refreshRoomPayload or roomPayloadRefresh.

Show Logs

Paste the full output logs here with WECHATY_LOG=silly set

{"log":"13:34:09 INFO RoomAdd JoinGroupByKeyword: matchList:[{\"_id\":\"5b2ca6247f15d37844fbdb34\",\"projectId\":\"5aa5f46d66f3e250590e4374\",\"topic\":\"测试群\",\"keyword\":[\"623\"],\"welcome\":[\"5b065af4b5a89f4456a79994\"],\"rule\":[\"5b06626cb5a89f4456a799a0\"],\"roomFull\":[],\"roomMaxNum\":\"400\"}]\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638175614Z"}
{"log":"13:34:09 VERB Room find({\"topic\":\"测试群\"})\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638191051Z"}
{"log":"13:34:09 VERB Room findAll()\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638196402Z"}
{"log":"13:34:09 VERB Puppet roomSearch({\"topic\":\"测试群\"})\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638200151Z"}
{"log":"13:34:09 VERB PuppetPadchat roomList()\r\n","stream":"stdout","time":"2018-06-23T05:34:09.63820536Z"}
{"log":"13:34:09 VERB PuppetPadchatManager getRoomIdList()\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638210058Z"}
{"log":"13:34:09 VERB PuppetPadchatManager getRoomIdList()=43\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638430769Z"}
{"log":"13:34:09 SILL PuppetPadchat roomList()=43\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638438604Z"}
{"log":"13:34:09 SILL Puppet roomSearch() allRoomIdList.length=43\r\n","stream":"stdout","time":"2018-06-23T05:34:09.638724878Z"}
{"log":"13:34:09 VERB Puppet roomQueryFilterFactory({\"topic\":\"测试群\"})\r\n","stream":"stdout","time":"2018-06-23T05:34:09.64150752Z"}
{"log":"13:34:09 SILL Puppet roomSearch() roomIdList filtered. result length=0\r\n","stream":"stdout","time":"2018-06-23T05:34:09.641510447Z"}
{"log":"13:34:09 VERB Contact say(没有找到群测试群, 请联系管理员!)\r\n","stream":"stdout","time":"2018-06-23T05:34:09.693844926Z"}
{"log":"13:34:09 VERB PuppetPadchat messageSend({\"contactId\":\"skz4204646520\"}, 没有找到群测试群, 请联系管理员!)\r\n","stream":"stdout","time":"2018-06-23T05:34:09.693855606Z"}
{"log":"13:34:09 SILL PadchatRpc rpcCall(WXSendMsg, [\"skz4204646520\",\"没有找到群测试群, 请联系管理员!\",\"\"])\r\n","stream":"stdout","time":"2018-06-23T05:34:09.693859019Z"}
{"log":"13:34:10 ERR RoomAdd JoinGroupByKeyword ERROR: XX未能成功入群\r\n","stream":"stdout","time":"2018-06-23T05:34:10.00918157Z"}
{"log":"错误原因:没有找到该群:测试群\r\n","stream":"stdout","time":"2018-06-23T05:34:10.009195352Z"}
@huan

This comment has been minimized.

Copy link
Member

huan commented Jun 23, 2018

Please have a look at:

await this.roomPayloadDirty(roomId)
await this.roomMemberPayloadDirty(roomId)
// XXX: Do we need to re-load payload at here?

and pay attention to my comment: // XXX: Do we need to re-load payload at here?

I agree with your analysis of this problem, and this case was considered before.

My suggestion would be: add the following three lines right after the comment to reload the payload for you:

    await new Promise(r => setTimeout(r, 1000))
    await this.roomPayload(roomId)
    await this.roomMemberPayload(roomId)

If that could fix your issue, a PR will be welcome.

@huan huan added the bug label Jun 23, 2018

@windmemory

This comment has been minimized.

Copy link
Contributor

windmemory commented Jun 24, 2018

What's the point to remove the room information after a member added to the room? If that is aim to prevent data get out of date, how about just do a refresh each time after a member get into the room? Same for roomTopic function, if room information get changed, how about just reload the information instead of remove it and then get it back?

@huan

This comment has been minimized.

Copy link
Member

huan commented Jun 24, 2018

What's the point to remove the room information after a member added to the room?

We need to update the RoomMemberList after a Room.add() operation. Or Room.has() will not be able to know this new user is in this room.

how about just do a refresh each time after a member get into the room?

Refresh = Dirty + Payload, as in my suggestion, what you should do is:

await this.roomMemberPayloadDirty(roomId)
await this.roomMemberPayload(roomId)
@windmemory

This comment has been minimized.

Copy link
Contributor

windmemory commented Jun 24, 2018

To my understanding, if flash-store works same as a Map, then with this piece of code in padchat-manager -> roomRawPayload

if (tryRawPayload /* && tryRawPayload.user_name */) {
  this.cacheRoomRawPayload.set(id, tryRawPayload)
  return tryRawPayload
}

This will override the old data in the cache, so we can just do

await this.roomPayload(roomId)

instead of do a dirty then get the data.

What do you think?

@huan

This comment has been minimized.

Copy link
Member

huan commented Jun 24, 2018

Yes, your understanding of the flash-store is right.

However, padchatManager.roomRawPayload() will never be called if the cache is not dirtied.

Please read the following code to learn more:

/**
* 1. Try to get from cache first
*/
const cachedPayload = this.roomPayloadCache(roomId)
if (cachedPayload) {
return cachedPayload
}

As you can see: in the base class Puppet, if the cache exists, the cached data will ve returned directly, which will prevent you to override the old data in the cache.

And that's what the cache should be acting as.

@windmemory

This comment has been minimized.

Copy link
Contributor

windmemory commented Jun 24, 2018

Okay, that makes sense. Will submit PR later.

BTW, I think it makes sense to have another method to force refresh a specific item in cache.

@huan

This comment has been minimized.

Copy link
Member

huan commented Jun 24, 2018

Great, looking forward to your PR.

BTW: I'm afraid that I could not agree with you about the propose of adding a new method because it will violate the philosophy of Unix:

building small programs that do one thing, do it well and compose easily with other programs.

I hope you will agree with me about that, thanks.

@windmemory

This comment has been minimized.

Copy link
Contributor

windmemory commented Jun 24, 2018

I would say if a piece of code you wrote used multiple places, then it makes sense to wrap it into a function and then reuse it everywhere. What I observed in the code base is that refreshing the data is useful in multiple places, so I think it would be better to have a refresh function, then reuse it, instead of write it multiple places and multiple times. (I would consider everywhere that do a dirty, then get the payload is refreshing the data). Even it might be a two-line function, in the future if you need to add more things when doing the refresh, you could add code in the function, then make the change affected all the places.

I agree with you about building small program philosophy, but this is not about the most fundamental components, but higher level code blocks, so I can not agree with the code. But I do respect everything in this repository, and will obey all the rules :)

@huan

This comment has been minimized.

Copy link
Member

huan commented Jun 24, 2018

Thanks for your respective.

Two more things:

  1. After puppet-padchat is promoted to a solo npm module and moved to the new repository, you will be able to refactor this module whatever as you want.
  2. Sometimes we have to only "dirty" the cache instead of refresh it. I believe you can find it later.
@windmemory

This comment has been minimized.

Copy link
Contributor

windmemory commented Jun 24, 2018

Sometimes we have to only "dirty" the cache instead of refresh it. I believe you can find it later.

Yes, I do see places that only "dirty" the cache. Actually I am not standing against the "dirty" functions. What I am suggesting is to have one more function to do the refresh job.
But anyway, just a suggestion, you make the call :)

@huan

This comment has been minimized.

Copy link
Member

huan commented Jun 24, 2018

@windmemory The PR was merged and please close this issue after you confirm this bug was fixed.

Thank you very much!

@windmemory

This comment has been minimized.

Copy link
Contributor

windmemory commented Jun 24, 2018

Great, thanks! Will check.

@huan

This comment has been minimized.

Copy link
Member

huan commented Jul 3, 2018

@windmemory Cloud you please close this issue if the problem had been solved?

@windmemory

This comment has been minimized.

Copy link
Contributor

windmemory commented Jul 5, 2018

Seems like the issue is resolved. Close this issue.

@windmemory windmemory closed this Jul 5, 2018

@huan

This comment has been minimized.

Copy link
Member

huan commented Jul 5, 2018

Thank you for the confirmation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment