Skip to content
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

Notify first instance that another instance has started #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jorisvergeer
Copy link
Contributor

Hi,

I wonder what you think about this. This patch created a named pipe server with the same name as the mutex when the first instances gets the lock. Any other instances that do not receive the lock connect to the named pipe and send a magic command. The first instance can react to this.

Handy when you want to let your MainWindow pop to the front for example.

@Lakritzator
Copy link
Member

Lakritzator commented Mar 6, 2020

Hi Jonas,

That functionality makes sense, it's similar to what I use in Greenshot where one can use the command line to inform the running instance.
Here is the server and the client. I used an interface to define how they communicate.
It used WCF, so with dotnet core it doesn't work, but maybe it gives you an idea.

It's quite a lot of code, and I only looked at the first 2 files.

As soon as I merge it I will be responsible for it, so I need to read through it to see if I would do it similar, or what you did nicer than I would have. So I will need to check it out, I already warned your that I'm tired tonight, so it needs to wait until I am alone and got energy, can take up to a week 🤷‍♂

Best wishes,
Robin

@jorisvergeer
Copy link
Contributor Author

jorisvergeer commented Mar 8, 2020

I do have some remark about my own code you might want to consider.

  1. I used the same identifier as the mutex, so the mutex id name is also used as named pipe name.
  2. Someone might want to have an option to opt-in to use the named pipe
    3, I am not completely sure if all exception handling is done right/acceptable.

Joris

@Lakritzator
Copy link
Member

Lakritzator commented Mar 11, 2020

I still didn't have much time to look at it, my wife and kid are ill (not Corona) and I'm slightly tired from getting up multiple times at night.

I just wanted to say the following: It would be really cool if there is a way to use an interface to communicate with the other instance. That would be a lot like WCF though...

Btw. I also couldn't start Visual Studio this evening, whatever reason... there was a lot of processes in the background which seemed to be an update, but it wasn't. My PC is slightly old, and the SSD is the worst ever... slow as hell... so I need to wait for it to finish 😢

/// <param name="namedPipeId">ID of the named pipe, preferably a Guid as string</param>
/// <param name="resourceName">Name of the resource to lock, e.g your application name, useful for logs</param>
/// <param name="global">true to have a global named pipe see: https://msdn.microsoft.com/en-us/library/bwe34f1k.aspx</param>
public static ResourceNamedPipeServer Create(ILogger logger, string namedPipeId, string resourceName = null, bool global = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I like to enforce the complexity of the Mutex, with global/local in this class. It should know as little as possible.

using System.Text;
using Microsoft.Extensions.Logging;

#if NET472
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The project only supports dotnet core 3.1, is there a reason for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I just noticed, it's a left-over from me 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... i should have removed that.

@Lakritzator
Copy link
Member

Hey Joris,

I hope all is well?!

I still think the idea for this feature is good, I would prefer it when it's not directly inside the mutex code. And it should be an option, which needs to be enabled.

There are some other projects which might help you get some ideas:
What about this project? https://github.com/jacqueskang/IpcServiceFramework
OR this: https://github.com/cyanfish/grpc-dotnet-namedpipes

Best wishes,
Robin

@jorisvergeer
Copy link
Contributor Author

jorisvergeer commented Apr 16, 2020

Hi,

Sorry for my absence. I've been busy with work, which nowadays happens at home, and i am dealing with some deadlines. I'll try to reserve some hours to look into this again this week.

Joris

@Lakritzator
Copy link
Member

No need for apologies, take all the time you need.

@Lakritzator
Copy link
Member

Lakritzator commented Jun 2, 2020

Hi Joris, just checking up on you to see if you are up for it now. Don't feel pressured, just want to make sure you didn't forget this ;)

@jorisvergeer
Copy link
Contributor Author

It is still on my mind

But I am still working on other projects at the moment.

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2020

CLA assistant check
All committers have signed the CLA.

@jorisvergeer
Copy link
Contributor Author

@Lakritzator
Hi,

I have some time soon. I'll start with your current code base.

Do you want this feature to be separate from the mutex implementation?
I can also change the mutex implementation to use the named pipe, it 'll then do the same and more. But it won't be using mutexes anymore then.

@Lakritzator
Copy link
Member

Hi @jorisvergeer,

nice to see you back 👋

I think the mutex is a simple, effective and proven, way of detecting if the application is already running. I would leave that functionality as is.

If you think that it's possible that the named pipe solution has similar functionality, we might give the user this as an option. I could even imagine that, if your feature is more than just a few lines of code and needs additional dependencies, it would be another package.

Before you start, it might make sense to go over what you are going to implement.

Best wishes,
Robin

@jorisvergeer
Copy link
Contributor Author

Hi,

Good to hear from you again.

For NamedPipes, we don't need any other dependencies than the .net core or framework itself.

A named pipe can function as mutex as only one server can exist for a given named pipe. A named pipe can also be local to the current user as global for the entire local machine. (I don't see a useful need for a global named pipe in this library, but somewhere someone might have a valid need)

I understand that you want to keep the proven mutex functionality.
A possible integration I think of is that the named pipe feature is an optional feature of the current AppServices library. Then, when enabled, the named pipe server is started when the mutex is owned.
Then when another instance fails to own the mutex, then it can send a signal to the server with a named pipe client.
This is quite similar to what I had earlier I think.

Furthermore, I found a more appropriate object to send the signal. I was thinking about that there should be an object that was less complicated than a named pipe but could just send a signal. Then I found out that there are Named Events in windows and that EventWaitHandles are exactly that. And the API's are quite simple. It is windows only though. Not sure if that is a problem?

using System;
using System.Threading;

namespace NamedEventTest
{
    class Program
    {
        const string NamedEventName = "my_fancy_named_event";

        static void Main(string[] args)
        {
            if(EventWaitHandle.TryOpenExisting(NamedEventName, out var waitHandle)) {
                Console.WriteLine("Wait handle found, signaling");
                waitHandle.Set();
            }
            else
            {
                waitHandle = new EventWaitHandle(false, EventResetMode.AutoReset, NamedEventName);

                while(true) {
                    // Could be more fancy with some async task wait
                    if(waitHandle.WaitOne(TimeSpan.FromSeconds(5)))
                    {
                        Console.WriteLine("I was signaled");
                    }
                    else
                    {
                        Console.WriteLine("Still waiting for signal");
                    }
                }
            }

            Console.WriteLine($"Bye...");
        }
    }
}

@Lakritzator
Copy link
Member

I don't think named events are a good way of communicating, unless for synchronizing something.
They are in fact similar to mutexes from a functionality perspective, I now understany why you wanted to replace the mutex.

I was thinking something in the line of a named pipe, and transport serialized (json) data over it...

@jorisvergeer
Copy link
Contributor Author

Why do you want to transport serialized json, while the only thing you want to send is an signal from one application to another instance.

Or do you want to see a more advanced feature?

@Lakritzator
Copy link
Member

Damn, I think we should really have a short discussion about what we want to accomplice with this!

From my side, I would like to be able to inform the running process of certain things to do.
This should allow a user to start the process, and later use the same command to for instance supply a new file to process.
The new instance notices that there is already something running, and passes this towards that running instance, than exits.

I always prefer to have interfaces to develop again, for Greenshot I would have an interface describing what actions the running instance can handle. For instance "OpenFile(string pathToFile)", which would be called after the commandline arguments (e.g. "Greenshot --open filename") are parsed.

To allow interfaces to be used, I would expect a serialize/deserialize to take place.

Of course it might make sense to provide a much easier core functionality, where people can put their own layer upon. But I still need more than just an event, there needs to be a way to pass additional information).

I think you should explain your use-case, I can fully imagine that multiple functionalities are provided. Simple event based, or maybe a named-pipe with string content...

Hope this helps to understand what went on in my mind 😄

@jorisvergeer
Copy link
Contributor Author

My use case is that I just want a single instance and to activate the first instance.

But now that you said it, the use case with open file is quite common. And I could see some convenience features in my application where I can create multiple shortcuts that open the correct page in my application.

@jorisvergeer
Copy link
Contributor Author

Suppose we build this.
Don't we then just end up with something similar to this library you mentioned earlier? https://github.com/jacqueskang/IpcServiceFramework

@Lakritzator
Copy link
Member

I think you are right, that should cover pretty much what we need...
So, what do you think? It was nice talking about this, we can close the PR or do you still have an urge to follow this through?
I can image at least a sample for this?

P.S.
At first I didn't understand how client could find the server, as the pipe name in the client & server don't match, but it's just the readme.md... I just created a PR for this.

@jorisvergeer
Copy link
Contributor Author

jorisvergeer commented Jun 25, 2020

It was productive. I've learned about named events.

One catch though. That library has separate client and server instances. I wonder what that library does when the client and server are both the same application, and wheather it can detect if a server is already present.

@Lakritzator
Copy link
Member

Lakritzator commented Jun 25, 2020

I think it would be good to have a sample / demo for this, if I find some time I will have a go.
I will create an issue for this, and close the PR, you okay with this?
Or would you still like to have the named events solution, it's not that big I guess...

@jorisvergeer
Copy link
Contributor Author

That is fine for now.

I currently have this initial pull request in my application and that works fine. It would be nice if this functionality is somewhere in a nuget package. But it is good for now.

If the solution with IpcServiceFramework does not work (or if hacks are needed), then we can reopen this again.

@Lakritzator
Copy link
Member

Like I said, I have no problem merging something, if there is a use case for it.
It seems you need it, are you talking about the current PR state or about the named event?

Why not modify your current code, and enable it to allow to transport an arbitrary blob of data or string?
Preferably something which is separated into an event (name) and a payload? This would than be the base where others can continue on. From there on, it would be easy to serialize & deserialize or just transport an image and such.

I had a quick look at the dependencies for the IpcServiceFramework, but having Castle.Core in my application is another logger abstraction and dynamic proxy, not really something I am waiting for...

Just a suggestion.

@jorisvergeer
Copy link
Contributor Author

I'll try if I can come up with something more appropriate than what I had before, still using named pipes but separated form the mutex code.

@jorisvergeer
Copy link
Contributor Author

Hi,

Without having written anything functional.

How do you like something like this as user API?

                .ConfigureSingleInstance(builder =>
                {
                    builder.MutexId = "{C3CC6C8F-B40C-4EC2-A540-1D4B8FFFB60D}";
                    builder.WhenNotFirstInstance = (hostingEnvironment, logger) =>
                    {
                        if(builder.HostBuilder.TryGetNamedPipeClient(out var client))
                        {
                            client.TrySendJson(new { Message = "NEW_INSTANCE" });
                        }
                        // This is called when an instance was already started, this is in the second instance
                        logger.LogWarning("Application {0} already running.", hostingEnvironment.ApplicationName);
                    };
                })
                .ConfigureNamedPipeServer(builder =>
                {
                    builder.NamedPipeId = "{C3CC6C8F-B40C-4EC2-A540-1D4B8FFFB60D}";
                    builder.WhenMessageReceived = (message, provider) =>
                    {
                        provider.GetService<MyMessageHandler>().Handle(message);
                    };
                    builder.WhenServerAlreadyExists = () =>
                    {
                    };
                })
                .ConfigureNamedPipeClient(builder =>
                {
                    builder.NamedPipeId = "{C3CC6C8F-B40C-4EC2-A540-1D4B8FFFB60D}";
                    builder.WhenServerDoesNotExists = () =>
                    {
                    };
                    builder.WhenServerDisconnected = () =>
                    {
                    };
                })

In this example the first instance becomes the server and also has the client initialized and available. Any other instance only has the client available. In this example the other instances use the existing single instance function to send a message to the server.

Still liking the idea to use an existing Ipc library more TBH.

@Lakritzator
Copy link
Member

Lakritzator commented Jul 6, 2020

I'm not so sure about "TrySendJson".

I guess "TrySendMessage" should be ok, this keeps the API agnostic and doesn't force a certain message format.
With that the developer can choose what he likes, use JSON (with his own serializer), protobuf, or maybe even a BinaryFormatter, although the later is not recommended but that is their choice!

We can provide signatures for the the message being:

  • byte[]
  • Span<byte>
  • string
  • stream

And as it's a "Try..." it should probably return a bool, specifying if the other side received the message.

What about the other side returning an answer?
I don't have a use-case which needs this right now, but that shouldn't be that hard right?

@jorisvergeer
Copy link
Contributor Author

Hi, as discussed earlier, IpcServiceFramework was an option (especially now that castle.core is gone). I tried it and it works.

I created an example that that works with the SingleInstance but I had to change the signature of WhenNotFirstInstance to allow access to the IServiceProvider and to enable it to run async calls.

The changes are in this separate branch:
jorisvergeer@566c486

I only created the example in WpfDemo.

@jorisvergeer
Copy link
Contributor Author

Thinking about this now,

Instead of a extended WhenNotFirstInstance signature. Another thing to potentially allow something async to happen is to register something like a WhenNotFirstInstanceHandler class in the builder.

@jorisvergeer
Copy link
Contributor Author

Last alternate example is here
jorisvergeer@a603d32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants