[Discussion] Ways to improve multi-thread handling beyond the current possibilites about fluentftp.gnutls HOT 27 CLOSED

robinrodricks/FluentFTP#1273

Do I understand correctly that there will only be one parallel upload/download at a time?

Edit: From the gnutls manual I think it should be possible to do parallel sessions. Is there anything in this library that would prevent that?

from fluentftp.gnutls.

So now we have the following status:

Per default, dll librarys are loaded on first use, and unloaded when stream disposed and use count is 0.

This behaviour, as default, works fine for single thread use and for multi-threaded concurrent use.

A user, who is observing too many library unloads/re-loads on account of his special use case, can disable unloading (and thus also the re-loading) of the libraries by using the special config variable DllUnloadByUseCount (default is true) and setting this to false.

Why might a user observe too many library unloads/re-loads and why might it be desirable to disable this?

1. Long running program performing ftp requests, constantly loading, transferring and then unloading until the next request comes in.
2. Single thread program transfering very many single files while connecting/disconnecting for each file.

a.) Worried about possible memory leaks
b.) Worried about performance penalty of unload/re-load process

from fluentftp.gnutls.

released 1.0.21 with the above changes.

1.0.21

• Implement a use-count based load/unload logic for the .dll libraries (thanks @Jojo-1000)
• Add logging for load and unload library actions
• Add a config parm DllUnloadByUseCount to disable use-count based unloading
• Remove the interim experimental parm DeInitGnuTLS that had a similar function
• Improve cred and sess dispose handling (thanks @Jojo-1000)

We will see where this takes us. Please provide input as your schedules allow and thanks for the cooperation and input thus far.

from fluentftp.gnutls.

Hello,

I have tried 1.0.21 without setting DllUnloadByUseCount to false, leaving it being true: fatal crashes are back. After some ten hours of polling a FTP server every minute, disposing of the FTP client at each time, the service crashed again without any exception logged anywhere (neither in our log nor in Windows events logs), just a hard kill of the process. (System events log states the service has unexpectedly stopped. No multi-threaded use of a FTP client was involved in this test.) (Now a second crash occurred 14 hours later.)

So, there is still an issue when repeatedly initializing/de-initializing GnuTLS. (Of course, I will go on deactivating this feature for production use. Or maybe I could change the code of that service to not release the FTP client but just close/re-open it.)

(Although this trouble is actually not multi-thread related, since we are discussing the init/deinit subject here, I have not opened another issue.)

from fluentftp.gnutls.

Yeah, there's a config that you can set to tell him: don't dispose for when you need thread safety. Go through the recent closed issues, you'll find it.

from fluentftp.gnutls.

No. Potentially thousands in parallel

from fluentftp.gnutls.

That is great to hear.

In our application (backup software) there can be a long time between connections. It would be nice to close the gnutls library to use fewer resources.

I think your init/deinit problem can be solved by reference counting the number of open users. Then the option would no longer be necessary and there is no open library.

If that sounds good, I can try and make a pull request.

from fluentftp.gnutls.

@Jojo-1000

The FluentFPT.GnuTLS "extension" was prompted by a flurry of problems that surfaced last year or even earlier. No need to reiterate them here.

I completed most of the coding (we are not yet at 100%, by far) in the first quarter of this year. Even with some features missing it was deemed a good idea to release this and work onwards depending on feedback that is coming in.

There has not been much such feedback, so there is no real indication how many users are actually profiting from this. As usual, you only hear from the community out there when something goes wrong.

The thread-safety issue for parallel use has come up only once previously, you are the second such reporter. By the way, it was not surprising, the limitations of the initial code were clear.

So: right now, we are at the stage that we hope for more feedback on "it now works in a parallel-usage environment if you set this parameter", and to enable that, we have initially created that rather strange new config parameter.

In the end, your suggestion of a thread-safe "in-use" counter is (probably) the best way to go. If you want to open a PR for that, I would be enthusiastic to see that happening.

Concerning the resource use of "keeping the lib initialized", there is actually not a huge footprint to worry about using the current solution - just like I would worry about repeated de-inits followed by re-inits with a usage count solution. Maybe one would in the end offer multiple possible solutions for this problem depending on the usage szenario of the user.

from fluentftp.gnutls.

just like I would worry about repeated de-inits followed by re-inits with a usage count solution.

Maybe instead of a direct use count, it could be run in the destructor/finalizer of an object. That way the library would be closed when the garbage collector runs, so I think there is some delay between using and closing it. But I am not sure how C# garbage collection works in detail.

from fluentftp.gnutls.

I understand your idea.

Hmmm.

First thought would be: Let's offer to the user the following options:

1. Single thread operation. Init and De-Init bracket all uses of the single GnuTlsStream

It has the advantage of being linear in operation and good for debugging a single usage of the stream.

2. Multi-thread operation with no De-Init

Cleanest way to ensure trouble free thread safety.

3. (Future) Multi-thread operation by way of a "use-count"

Less resource usage, (possibly) more overhead

4. (Future) Externalize Init and De-Init in some way for the user to execute as and when he sees fit.

That's actually the way the base GnuTLS library does it: By giving you full control over when and how you Init and De-Init the lib.

from fluentftp.gnutls.

3. would be a regression for my use case. It is a background service polling a FTP server every minute. With de-init enabled, some kind of resource leak seems to occurs (handles count increasing) and the service is abruptly killed up to two times a day. (No exception logged, just an abnormal service stop log in the Windows event system log.)

(For completeness, this service also do some uploads, most guaranteed to not occur during polling, but a few others may occur in another thread during polling. This service also uses the async FTP client.)

I think 4. should be done, maybe through some static class/method, or an initializer/deinitializer class to instantiate and run once.

And then do some breaking change to avoid surprising pitfalls in multi-threaded context: no more on-the-fly init if not explicitly initialized, but an InvalidOperationException stating the lib needs an explicit initialization. Eventually a less breaking change could be done instead: switch deinit to false by default.

Being exposed to thread safety troubles by default is a flaw in my opinion. The library should not stay in that situation.

from fluentftp.gnutls.

From the docs of global_init, it is not necessary to call it at all since GnuTLS 3.3. It also states that there is internal reference counting for init/deinit and it is thread safe. So to fix the crash I think the only change would need to be removing the if intialized checks in the GnuTLS stream and always call init/deinit. Then you can still use the option to keep the library initialized if you prefer, but it would hopefully no longer crash by default.

Edit

I propose this:

--- a/GnuTlsInternalStream.cs
+++ b/GnuTlsInternalStream.cs
@@ -134,6 +134,11 @@ namespace FluentFTP.GnuTLS {

                                        weAreInitialized = true;
                                }
+                               else if (deInit) {
+                                       // This is usage counted, so global init only occurs once, even if it is called multiple times
+                                       // Call init once for every opened stream, and deinit when it is disposed
+                                       GnuTls.GnuTlsGlobalInit();
+                               }
                        }

                        // Setup/Allocate certificate credentials
@@ -211,9 +216,9 @@ namespace FluentFTP.GnuTLS {

                        cred.Dispose();

-                       if (weAreControlConnection && deInit) {
+                       if (deInit) {
+                               // Global deinit only occurs once it is called the same number of times as global init
                                GnuTls.GnuTlsGlobalDeInit();
-                               weAreInitialized = false;
                        }

                }

from fluentftp.gnutls.

Thanks for the comments/inputs.

Is there anything keeping you from testing/implementing these ideas?

In the next days, I will play with this also. It is correct that from GnuTLS 3.3 onwards, a repeated Init call is "ignored". And that de-inits are supposed to be handled in a graceful fashion.

The problem lies in the "cred.dispose", possibly, especially in the context of sessions? I will look at it in more detail.

Also please don't forget that a final Marshalling-don't need anymore-this dll might just pull the rug out from the whole thing.

But until now, I think you are on the right track to worry about the init-de-init methods and how they might "go wrong". At least the might need to be "serialized" so that they are not invoked in parallel?

from fluentftp.gnutls.

Being exposed to thread safety troubles by default is a flaw in my opinion. The library should not stay in that situation

I agree. I invested a LOT of time to get it to where it is now (from zero) and I appreciate any help in this direction that may be available. My personal use cases do not extend to multi-threaded operations, so I paid less attention to that beyond basic measures.

from fluentftp.gnutls.

Unfortunately it seems that my changes are not enough to fix the crash. There also seems to be multiple points where the crash occurs, sometimes in a Read() with an expired session pointer, and sometimes in Credentials.Dispose with an expired credentials pointer. Since I have the build set up now, I hope I will be able to fix this in the next days.

Edit

That was easier than I thought, it was not the session/credentials pointer that was expired, but rather the gnutls function pointer, because the GnuTlsGlobalDeInit() wrapper also frees all functions.

from fluentftp.gnutls.

Ok, so now let's play with this and stress test it. Nice changes - splendid that we can get on with this topic. Thanks!

from fluentftp.gnutls.

I've decided to utilize the "use-count" logic all the time. No need for the config variable DeInitGnuTls anmore. Those of you who have been using it, will get a compile error and can just remove it.

I know I stated this earlier:

First thought would be: Let's offer to the user the following options:
Single thread operation. Init and De-Init bracket all uses of the single GnuTlsStream
It has the advantage of being linear in operation and good for debugging a single usage of the stream.
Multi-thread operation with no De-Init
Cleanest way to ensure trouble free thread safety.
(Future) Multi-thread operation by way of a "use-count"
Less resource usage, (possibly) more overhead
(Future) Externalize Init and De-Init in some way for the user to execute as and when he sees fit.

But this has the least complexity... and if someone now wants, for some special reason, to disable possible unloading and re-loading*1), it will be easier to bring that in. All we need to give him is a Pre-Load method, which if he uses it will permanent-load the libs, and we give him an explicit way to free the library when he decides he no longer wants it. This will be a next step, perhaps.

*1) This unloading and re-loading will happen for non-multi-threaded sequential transfers with connect-disconnect sequence (same or differing hosts).

from fluentftp.gnutls.

Thanks for your effort on this topic, but as I told, the use-count logic without the de-init option for disabling it will likely cause a serious regression in my case.

So, I reformulate: currently, initializing / de-initializaing the GnuTLS library a lot of times causes some resources leaks ending up fatal-crashing the process (catch blocks not even executed). At least that is what happens in the case of my application. Deactivating the de-initialization has fixed these crashes for my application.

from fluentftp.gnutls.

@fredericDelaporte Ok, don't worry. I am already working on providing an option to "continuously" keep the libs loaded.

It is just easier to code it "this way around".

The option you need will arrive soon.

from fluentftp.gnutls.

Ok, thanks.

from fluentftp.gnutls.

@fredericDelaporte And also, you have not tested the current status, @Jojo-1000 had fixed a few loopholes in the way Dispose was coded. So you cannot be sure that the failure you described actually still happens.

from fluentftp.gnutls.

@fredericDelaporte

I did say:

and if someone now wants, for some special reason, to disable possible unloading and re-loading*1), it will be easier to bring that in

from fluentftp.gnutls.

Jojo-1000 had fixed a few loopholes in the way Dispose was coded

I have read that, yes, thus my "likely" wording. But checking if these loopholes were the reason for the fatal-crash cannot be done easily, since it involves running the application many days to ascertain it does not fatal-crash occasionally again. So, I had preferred to attempt to be on the safe side by voicing my preference for still being able to not have the library de-initialized each time all FTP clients are disposed of.

I did say:

Seen too, but for me it was not clear whether it would be done anyway or only if someone ends-up reporting troubles due to not having that possibility.

from fluentftp.gnutls.

@fredericDelaporte I understand. All is good. Working on it. I want it to be "elegant", better than the previous rather hurried solutions, now that we have a good handle on the logic.

from fluentftp.gnutls.

		/// <summary>
		/// The FluentFTP.GnuTLS extension to FluentFTP will correctly handle multi-threaded
		/// concurrent operation by maintining a "use-count", just like the underlying GnuTLS
		/// library does. This use-count is checked on dispose of the GnuTlsStream and an unload
		/// of the GnuTls library .dlls only takes place when this value is zero.
		/// To disable unloading and to keep the libraries permanently loaded, set
		/// DllUnloadByUseCount to false.
		/// </summary>
		public bool DllUnloadByUseCount { get; set; } = true;

from fluentftp.gnutls.

So, there is still an issue when repeatedly initializing/de-initializing GnuTLS. (Of course, I will go on deactivating this feature for production use. Or maybe I could change the code of that service to not release the FTP client but just close/re-open it.)

Interesting. Thanks for doing this. Since it has nothing to do with multithread operations, we can test this (probably, even I can test it) separately and also, we will do it in a separate issue than this one. This one is "Multi-Thread", so to speak. I will open one soon.

from fluentftp.gnutls.

See #100

from fluentftp.gnutls.