Implement removing from filter by ws

[0xAleksaOpacic]

Description

This PR fixes the issue with closing ws connection.

Fixes EDGE-584

Changes include

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

• I have assigned this PR to myself
• I have added at least 1 reviewer
• I have added the relevant labels
• I have updated the official documentation
• I have added sufficient documentation in code

Testing

• I have tested this code with the official test suite
• I have tested this code manually

Manual tests

Subscribe to newBlockHeadEvent (https://github.com/Aleksao998/SubscribeToNewHeadEvent) and close the connection, you should not see errors (on develop), on this branch no error logs should appear

2022-05-30T14:59:00.022+0200 [ERROR] polygon.jsonrpc: Unable to write WS message, writev tcp 127.0.0.1:10002->127.0.0.1:64893: use of closed network connection
2022-05-30T14:59:00.022+0200 [ERROR] polygon.filter: Unable to process flush, writev tcp 127.0.0.1:10002->127.0.0.1:64893: use of closed network connection

[codecov]

Codecov Report

Merging #570 (02813a7) into develop (11ac0bf) will increase coverage by 0.08%.
The diff coverage is 68.37%.

❗ Current head 02813a7 differs from pull request most recent head 397ee22. Consider uploading reports for the commit 397ee22 to get more accurate results

@@             Coverage Diff             @@
##           develop     #570      +/-   ##
===========================================
+ Coverage    51.00%   51.09%   +0.08%     
===========================================
  Files          108      108              
  Lines        15662    15691      +29     
===========================================
+ Hits          7989     8017      +28     
- Misses        6999     7001       +2     
+ Partials       674      673       -1     

Impacted Files	Coverage Δ
consensus/ibft/ibft.go	39.83% <0.00%> (-0.30%)	⬇️
jsonrpc/dispatcher.go	53.04% <0.00%> (-0.37%)	⬇️
jsonrpc/jsonrpc.go	17.75% <20.00%> (-0.15%)	⬇️
jsonrpc/eth_endpoint.go	71.49% <77.77%> (+0.77%)	⬆️
jsonrpc/filter_manager.go	75.31% <80.68%> (+2.64%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[Kourin1996]

LGTM

[dbrajovic]

Tbh, I don't find it great that now you have to manage (add/remove) the same object in two places: filters and filterByWsConn. But I guess that's due to the technical debt pf this package.

Just curious: was there any chance to expand the wsConn interface with like an ID() string method, so that you could easily remove the filter from the filters map when calling dispatcher's RemoveFilterByWs(conn wsConn) ?

[0xAleksaOpacic]

Tbh, I don't find it great that now you have to manage (add/remove) the same object in two places: filters and filterByWsConn. But I guess that's due to the technical debt pf this package.

Just curious: was there any chance to expand the wsConn interface with like an ID() string method, so that you could easily remove the filter from the filters map when calling dispatcher's RemoveFilterByWs(conn wsConn) ?

Not sure I understand the second part.
filters map[string]filter
So filter map is a map where the key is filterID and the value is filter. This bug is due to when we are closing WS connection we are not removing this filter. So I had to have a way of finding the filter by wsConn and then removing it from the FILTER map.
Not sure if wsConn should know about filters. It's a basic web socket connection. FilterManager should handle everything related to the filters

[dbrajovic]

Not sure if wsConn should know about filters. It's a basic web socket connection. FilterManager should handle everything > related to the filters

Yes, exactly - you needed a way to remove a certain filter just by having its wsConn. But because you can't extract the ID based on wsConn, you keep another record of the same object.

The same filter object can be found in 2 places;

1st
2nd

This just adds to the technical debt of this package (which will be tackled in a separate issue).

Approving now. 👍

[zivkovicmilos]

Looks good now, thank you for resolving the nitpicks 🙏

This filter manager is trash and god willing we'll get a chance to take it out soon, but until then this will work.

Please see the comments I've left you 🙏

Looks good 💯

[Kourin1996]

LGTM

[Kourin1996]

LGTM