SAP / node-rfc

Asynchronous, non-blocking SAP NW RFC SDK bindings for Node.js
Apache License 2.0
251 stars 73 forks source link

Track handle status #149

Closed clausreinke closed 4 years ago

clausreinke commented 4 years ago

Trying to fix (or at least improve) #128 and #147, based on the theory that the 1:1 relation between Clients and connectionHandles gets messed up (by an explicit or implicit close), at which point the Client mutexes no longer protect against multiple threads using the same connectionHandle. Main example: when the Client destructor closes a handle that has already been closed in that Client but has since been reopened by another Client.

This PR is possibly overly paranoid in some cases and most likely does not close all holes yet, but here is what it tries to do:

  1. avoid closing a handle in the Client destructor if the Client may have lost exclusivity for that handle
  2. try to tighten the management of Client->alive to more accurately track the handle status (reduce the time windows where multiple threads might see incorrect values)
CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

clausreinke commented 4 years ago
bsrdjan commented 4 years ago

Thank you very much for this contribution @clausreinke !

Based on tests so far, the RFC_INVALID_HANDLE does not occur any more in invalid_handle tests on: Ubuntu and Windows with SDK PL6, Darwin with SDK PL5. Still need to include this test into unit tests suite.

I only changed the handle pointer hex notation in fprintf logs to decimal notation, used in NWRFC SDK traces.

I expect more work and more analysis might be needed here, because frameworks out of client application control (express...) may spawn concurrent threads, consuming the Pool, not 100% prepared for such scenario. Let test more and investigate possible solutions.