Did not send eth_unsubscribe to the node when unsubscribing

[samlior]

Expected behavior

Send eth_unsubscribe to the node when unsubscribing

Actual behavior

Did not send eth_unsubscribe to the node when unsubscribing

Steps to reproduce the behavior

const subscription = web3.eth.subscribe("newBlockHeaders", (err) => {
    if (err) {
        console.log('err:', err)
    }
})
subscription.on('connected', () => {
    subscription.unsubscribe();
})

Environment

npm 7.7.4
node 12.19.0
web3-core-subscriptions@1.3.5
web3-core-requestmanager@1.3.5

Reason

// web3-core-subscriptions/src/subscription.js line279 (function subscribe)
this.options.requestManager.send(payload, function (err, result) {
    if(!err && result) {
        _this.id = result;
        _this.method = payload.params[0];
        _this.emit('connected', result);

        // call callback on notifications
        _this.options.requestManager.addSubscription(_this, function(error, result) {
            //...
        });
    } else {
        //...
    }
});

// web3-core-requestmanager/src/index.js line249 (function removeSubscription)
if (this.subscriptions.has(id)) {
    var type = this.subscriptions.get(id).subscription.options.type;

    // remove subscription first to avoid reentry
    this.subscriptions.delete(id);

    // then, try to actually unsubscribe
    this.send({
        method: type + '_unsubscribe',
        params: [id]
    }, callback);

    return;
}

It seems that when removeSubscription, it will find the subscription object from the this.subscriptions according to the id, but when the connected event is emitted, the object has not been added to the this.subscriptions. Therefore, when unsubscribe is called in the connected event, eth_unsubscribe will not be sent.

Suggest

Call requestManager.addSubscription before emit connected event.

[spacesailor24]

Thank you for bringing this to our attention. Currently our focus is on rewriting the library (v4.x), so we may not get to fixing this soon enough for you. If possible, you are invited to open up a PR fixing this issue! Otherwise, we'll keep this in mind when doing the rewrite and will verify the issue doesn't persist

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[samlior]

Can you review this pr? #4058 @spacesailor24

[philknows]

Hi, the actions are failing @samlior. If you can fix this, one of our team members will review. Thanks.

[nazarhussain]

As the actual commit in the referenced PR is merge to 1.x. So I am closing this issue.