Fix log collisions (RTVS) + other fixes #542
Conversation
…, Intellisense and test processes. Include host process name and VS process ID in log file names to avoid collisions. Fix IRSession.Current changing randomly when new sessions are created. Add checks to FileLogWriter to ensure there's only one instance of it for any given filename.
|
OK. I'll undo my log fix then :-) BTW, we can remove plot window IntPtr. |
| [Export(typeof(IRSessionProvider))] | ||
| public class RSessionProvider : IRSessionProvider { | ||
| private readonly ConcurrentDictionary<int, IRSession> _sessions = new ConcurrentDictionary<int, IRSession>(); | ||
| private readonly object _lock = new object(); | ||
| private readonly Dictionary<int, IRSession> _sessions = new Dictionary<int, IRSession>(); |
There was a problem hiding this comment.
ConcurrentDictionary.GetOrAdd
Though this code requires bigger refactoring, it isn't part of this change.
There was a problem hiding this comment.
The lock is there for Current as well as dictionary. And so long as we have the lock, it might as well be a regular dictionary.
There was a problem hiding this comment.
But why do you need it for Current?
Here is the whole method how I see it:
public IRSession Create(int sessionId, IRHostClientApp hostClientApp) {
var session = _sessions.GetOrAdd(sessionId, new RSession(sessionId, hostClientApp));
if (Current == null) {
Current = session;
CurrentChanged?.Invoke(this, EventArgs.Empty);
}
return session;
}
Current can change only if it is null, otherwise it remains the same. GetOrAdd works as a barrier - there will be no null for the same id twice. It is possible to get race for 2 different ids, but this case is currently undefined at all.
There was a problem hiding this comment.
Ok, forget it, I'll have to change this code soon
|
👍 |
| @@ -87,8 +90,10 @@ public sealed class FileLogWriter : IActionLogWriter { | |||
| } | |||
|
|
|||
| public static FileLogWriter InTempFolder(string fileName) { | |||
| var path = $@"{Path.GetTempPath()}/{fileName}_{DateTime.Now:yyyyMdd_HHmmss}.log"; | |||
| return new FileLogWriter(path); | |||
| return _writers.GetOrAdd(fileName, _ => { | |||
There was a problem hiding this comment.
Don't you need to use full file name as a key?
There was a problem hiding this comment.
If everyone goes through this function (which they now have to), no.
If we used the full key, then two components asking access to the same log would in most cases get two different files. As written, it will be the same file (and timestamp will be that of the first request). I think it's advantageous - this way you will generally end up with a single log per suffix per VS instance.
|
Can you also take a look at this? (which contains the necessary host changes) |
|
👍 |
|
Thanks! |
Fix log collisions (RTVS) + other fixes
Enable named host processes / sessions, and use this to separate REPL, Intellisense and test processes.
Include host process name and VS process ID in log file names to avoid collisions.
Fix IRSession.Current changing randomly when new sessions are created.
Add checks to FileLogWriter to ensure there's only one instance of it for any given filename.